Merge pull request #593 from keep-network/nested-revert-msg

fix: throw nested revert messages

FundingScript and RedemptionScript were including garbage data
in their reverts due to how they were capturing revert data from the
underlying contract calls they were making. This PR strips the extra
data on those internal reverts to present a single well-formed revert
message at the top level.

solidity/contracts/scripts/FundingScript.sol

@@ -50,11 +50,20 @@ contract FundingScript {
         // Call the VendingMachine.
         // We could explictly encode the call to vending machine, but this would
         // involve manually parsing _extraData and allocating variables.
-        (bool success, bytes memory returnData) = address(vendingMachine).call(
-            // solium-disable-previous-line security/no-low-level-calls
-            _extraData
-        );
-        require(success, string(returnData));
+        // We capture the `returnData` in order to forward any nested revert message
+        // from the contract call.
+        (bool success, bytes memory returnData) = address(vendingMachine).call(_extraData);
+
+        string memory revertMessage;
+        assembly {
+            // A revert message is ABI-encoded as a call to Error(string).
+            // Slicing the Error() signature (4 bytes) and Data offset (4 bytes)
+            // leaves us with a pre-encoded string.
+            // We also slice off the ABI-coded length of returnData (32).
+            revertMessage := add(returnData, 0x44)
+        }
+
+        require(success, revertMessage);
 
         // Transfer the TBTC and feeRebateToken to the user.
         tbtcToken.transfer(_from, tbtcToken.balanceOf(address(this)));

solidity/contracts/scripts/RedemptionScript.sol

@@ -48,11 +48,20 @@ contract RedemptionScript {
             "Bad _extraData signature. Call must be to tbtcToBtc."
         );
 
+        // We capture the `returnData` in order to forward any nested revert message
+        // from the contract call.
+        // solium-disable-next-line security/no-low-level-calls
         (bool success, bytes memory returnData) = address(vendingMachine).call(_extraData);
-        // solium-disable-previous-line security/no-low-level-calls
-        // By default, `address.call`  will catch any revert messages.
-        // Converting the `returnData` to a string will effectively forward any revert messages.
-        // https://ethereum.stackexchange.com/questions/69133/forward-revert-message-from-low-level-solidity-call
-        require(success, string(returnData));
+
+        string memory revertMessage;
+        assembly {
+            // A revert message is ABI-encoded as a call to Error(string).
+            // Slicing the Error() signature (4 bytes) and Data offset (4 bytes)
+            // leaves us with a pre-encoded string.
+            // We also slice off the ABI-coded length of returnData (32).
+            revertMessage := add(returnData, 0x44)
+        }
+
+        require(success, revertMessage);
     }
 }

solidity/test/VendingMachineTest.js

@@ -465,6 +465,33 @@ describe("VendingMachine", async function() {
         expect(success).to.equal(true)
       })
 
+      it("forwards nested revert error messages", async () => {
+        await testDeposit.setState(states.ACTIVE)
+        await tbtcDepositToken.forceMint(vendingMachine.address, tdtId)
+        await tbtcToken.forceMint(owner, depositValue.add(signerFee))
+        await feeRebateToken.forceMint(owner, tdtId)
+        const tbtcToBtc = vendingMachine.abi.filter(
+          x => x.name == "tbtcToBtc",
+        )[0]
+        const nonexistentDeposit = "0000000000000000000000000000000000000000"
+        const calldata = web3.eth.abi.encodeFunctionCall(tbtcToBtc, [
+          nonexistentDeposit,
+          "0x1111111100000000",
+          redeemerOutputScript,
+          owner,
+        ])
+
+        await expectRevert(
+          tbtcToken.approveAndCall(
+            redemptionScript.address,
+            depositValue.add(signerFee),
+            calldata,
+            {from: owner},
+          ),
+          "tBTC Deposit Token does not exist",
+        )
+      })
+
       it("reverts for unknown function calls encoded in _extraData", async () => {
         const unknownFunctionSignature = "0xCAFEBABE"
         await tbtcToken.forceMint(owner, depositValue.add(signerFee))
@@ -565,6 +592,39 @@ describe("VendingMachine", async function() {
       expect(success).to.be.true
     })
 
+    it("forwards nested revert error messages", async () => {
+      const unqualifiedDepositToTbtcABI = vendingMachine.abi.filter(
+        x => x.name == "unqualifiedDepositToTbtc",
+      )[0]
+      const calldata = web3.eth.abi.encodeFunctionCall(
+        unqualifiedDepositToTbtcABI,
+        [
+          testDeposit.address,
+          _version,
+          _txInputVector,
+          _txOutputVector,
+          _txLocktime,
+          _fundingOutputIndex,
+          _merkleProof,
+          _txIndexInBlock,
+          _bitcoinHeaders,
+        ],
+      )
+
+      // To make the funding call fail.
+      await testDeposit.setState(states.ACTIVE)
+
+      await expectRevert(
+        tbtcDepositToken.approveAndCall.call(
+          fundingScript.address,
+          tdtId,
+          calldata,
+          {from: owner},
+        ),
+        "Not awaiting funding",
+      )
+    })
+
     it("reverts for unknown function calls encoded in _extraData", async () => {
       const unknownFunctionSignature = "0xCAFEBABE"