Merge pull request #11 from rsksmart/feature/GBI-1652

Feature/GBI-1652 - Fix security findings
rsksmart · Feb 7, 2024 · f77fb51 · f77fb51
2 parents 5b3391d + 5986c1f
commit f77fb51
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 21 deletions.
diff --git a/contracts/BtcUtils.sol b/contracts/BtcUtils.sol
@@ -120,19 +120,9 @@ library BtcUtils {
     /// @param outputScript the fragment of the raw transaction containing the raw output script 
     /// @return The content embedded inside the script
     function parseNullDataScript(bytes calldata outputScript) public pure returns (bytes memory) {
+        require(outputScript.length > 1,"Invalid size");
         require(outputScript[0] == 0x6a, "Not OP_RETURN");
-        require(
-            outputScript.length > 2 && outputScript.length < 85,
-            "Data out of bounds"
-        );
-
-        bytes memory message = new bytes(uint8(outputScript[1]));
-        for (uint8 i = 0; i < message.length; i++) {
-            // the addition of two is because the two first bytes correspond to
-            // the op_return opcode and the length of the message
-            message[i] = outputScript[i + 2]; 
-        }
-        return message;
+        return outputScript[1:];
     }
 
     /// @notice Hash a bitcoin raw transaction to get its id (reversed double sha256)
@@ -208,7 +198,10 @@ library BtcUtils {
         uint compactSizeBytes = 2 ** (maxSize - MAX_COMPACT_SIZE_LENGTH);
         require(compactSizeBytes <= MAX_BYTES_USED_FOR_COMPACT_SIZE, "unsupported compact size length");
 
-        uint64 result = uint64(calculateLittleEndianFragment(sizePosition + 1, sizePosition + compactSizeBytes, array));
+        // the adition of 1 is because the first byte is the indicator of the size and its not part of the number
+        uint64 result = uint64(
+            calculateLittleEndianFragment(sizePosition + 1, sizePosition + compactSizeBytes + 1, array)
+        );
         return (result, uint16(compactSizeBytes) + 1);
     }
 
@@ -222,7 +215,7 @@ library BtcUtils {
             "Range can't be bigger than array"
         );
         uint result = 0;
-        for (uint i = fragmentStart; i <= fragmentEnd; i++) {
+        for (uint i = fragmentStart; i < fragmentEnd; i++) {
             result += uint8(array[i]) *  uint64(2 ** (8 * (i - (fragmentStart))));
         }
         return result;

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@rsksmart/btc-transaction-solidity-helper",
-  "version": "0.0.2",
+  "version": "0.0.3",
   "description": "Solidity library with functions to work with Bitcoin transactions inside smart contracts",
   "main": "contracts",
   "files": [

diff --git a/test/BtcUtils.ts b/test/BtcUtils.ts
@@ -72,25 +72,25 @@ describe("BtcUtils", function () {
     const firstRawTX = "0x0100000001013503c427ba46058d2d8ac9221a2f6fd50734a69f19dae65420191e3ada2d40000000006a47304402205d047dbd8c49aea5bd0400b85a57b2da7e139cec632fb138b7bee1d382fd70ca02201aa529f59b4f66fdf86b0728937a91a40962aedd3f6e30bce5208fec0464d54901210255507b238c6f14735a7abe96a635058da47b05b61737a610bef757f009eea2a4ffffffff0200879303000000001976a9143c5f66fe733e0ad361805b3053f23212e5755c8d88ac0000000000000000426a403938343934346435383039323135366335613139643936356239613735383530326536646263326439353337333135656266343839373336333134656233343700000000";
     const firstTxOutputs = await BtcUtils.getOutputs(firstRawTX);
 
-    const firstQuoteHash = await BtcUtils.parseNullDataScript(firstTxOutputs[1].pkScript).then(ethers.toUtf8String);
+    const firstNullScript = await BtcUtils.parseNullDataScript(firstTxOutputs[1].pkScript);
     const firstDestinationAddress = await BtcUtils.parsePayToPubKeyHash(firstTxOutputs[0].pkScript, false);
     const firstValue = firstTxOutputs[0].value;
     const firstHash = await BtcUtils.hashBtcTx(firstRawTX);
 
     const secondRawTX = "0x01000000010178a1cf4f2f0cb1607da57dcb02835d6aa8ef9f06be3f74cafea54759a029dc000000006a473044022070a22d8b67050bee57564279328a2f7b6e7f80b2eb4ecb684b879ea51d7d7a31022057fb6ece52c23ecf792e7597448c7d480f89b77a8371dca4700a18088f529f6a012103ef81e9c4c38df173e719863177e57c539bdcf97289638ec6831f07813307974cffffffff02801d2c04000000001976a9143c5f66fe733e0ad361805b3053f23212e5755c8d88ac0000000000000000426a406539346138393731323632396262633966636364316630633034613237386330653130353265623736323666393365396137663130363762343036663035373600000000";
     const secondTxOutputs = await BtcUtils.getOutputs(secondRawTX);
 
-    const secondQuoteHash = await BtcUtils.parseNullDataScript(secondTxOutputs[1].pkScript).then(ethers.toUtf8String);
+    const secondNullScript = await BtcUtils.parseNullDataScript(secondTxOutputs[1].pkScript);
     const secondDestinationAddress = await BtcUtils.parsePayToPubKeyHash(secondTxOutputs[0].pkScript, true);
     const secondValue = secondTxOutputs[0].value;
     const secondHash = await BtcUtils.hashBtcTx(secondRawTX);
 
-    expect(firstQuoteHash).to.eq("984944d58092156c5a19d965b9a758502e6dbc2d9537315ebf489736314eb347");
+    expect(firstNullScript).to.eq("0x4039383439343464353830393231353663356131396439363562396137353835303265366462633264393533373331356562663438393733363331346562333437");
     expect(firstDestinationAddress).to.eq("0x6f3c5f66fe733e0ad361805b3053f23212e5755c8d");
     expect(firstValue).to.eq("60000000");
     expect(firstHash).to.eq("0x03c4522ef958f724a7d2ffef04bd534d9eca74ffc0b28308797d2853bc323ba6");
 
-    expect(secondQuoteHash).to.eq("e94a89712629bbc9fccd1f0c04a278c0e1052eb7626f93e9a7f1067b406f0576");
+    expect(secondNullScript).to.eq("0x4065393461383937313236323962626339666363643166306330346132373863306531303532656237363236663933653961376631303637623430366630353736");
     expect(secondDestinationAddress).to.eq("0x003c5f66fe733e0ad361805b3053f23212e5755c8d");
     expect(secondValue).to.eq("70000000");
     expect(secondHash).to.eq("0xfd4251485dafe36aaa6766b38cf236b5925f23f12617daf286e0e92f73708aa3");