Attacker can force the failure of transactions that use tryCatch

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L119-L123

Vulnerability details

Attacker can force the failure of transactions that use tryCatch

An attacker or malicious relayer can force the failure of transactions that rely on tryCatch() by carefully choosing the gas limit.

Impact

The tryCatch() function present in the AmbireAccount contract can be used to execute a call in the context of a wallet that is eventually allowed to fail, i.e. the operation doesn't revert if the call fails.

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L119-L123

119: 	function tryCatch(address to, uint256 value, bytes calldata data) external payable {
120: 		require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
121: 		(bool success, bytes memory returnData) = to.call{ value: value, gas: gasleft() }(data);
122: 		if (!success) emit LogErr(to, value, data, returnData);
123: 	}

EIP-150 introduces the "rule of 1/64th" in which 1/64th of the available is reserved in the calling context and the rest of it is forward to the external call. This means that, potentially, the called function can run out of gas, while the calling context may have some gas to eventually continue and finish execution successfully.

A malicious relayer, or a malicious actor that front-runs the transaction, can carefully choose the gas limit to make the call to tryCatch() fail due out of gas, while still saving some gas in the main context to continue execution. Even if the underlying call in tryCatch() would succeed, an attacker can force its failure, while the main call to the wallet is successfully executed.

Proof of Concept

The following test reproduces the attack. The user creates a transaction to execute a call using tryCatch() to a function of the TestTryCatch contract, which simulates some operations that consume gas. The attacker then executes the bundle by carefully choosing the gas limit (450,000 units of gas in this case) so that the call to TestTryCatch fails due to out of gas, but the main call to execute() in the wallet (here simplified by using executeBySender() to avoid signatures) gets correctly executed.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

contract TestTryCatch {
    uint256[20] public foo;

    function test() external {
        // simulate expensive operation
        for (uint256 index = 0; index < 20; index++) {
            foo[index] = index + 1;
        }
    }
}

function test_AmbireAccount_ForceFailTryCatch() public {
    address user = makeAddr("user");

    address[] memory addrs = new address[](1);
    addrs[0] = user;
    AmbireAccount account = new AmbireAccount(addrs);

    TestTryCatch testTryCatch = new TestTryCatch();

    AmbireAccount.Transaction[] memory txns = new AmbireAccount.Transaction[](1);
    txns[0].to = address(account);
    txns[0].value = 0;
    txns[0].data = abi.encodeWithSelector(
        AmbireAccount.tryCatch.selector,
        address(testTryCatch),
        uint256(0),
        abi.encodeWithSelector(TestTryCatch.test.selector)
    );

    // This should actually be a call to "execute", we simplify the case using "executeBySender"
    // to avoid the complexity of providing a signature. Core issue remains the same.
    vm.expectEmit(true, false, false, false);
    emit LogErr(address(testTryCatch), 0, "", "");
    vm.prank(user);
    account.executeBySender{gas: 450_000}(txns);

    // assert call to TestTryCatch failed
    assertEq(testTryCatch.foo(0), 0);
}

Recommendation

The context that does the call in tryCatch() can check the remaining gas after the call to determine if the remaining amount is greater than 1/64 of the available gas before the external call.

  function tryCatch(address to, uint256 value, bytes calldata data) external payable {
      require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
+     uint256 gasBefore = gasleft();
      (bool success, bytes memory returnData) = to.call{ value: value, gas: gasleft() }(data);
+     require(gasleft() > gasBefore/64);
      if (!success) emit LogErr(to, value, data, returnData);
  }

Assessed type

Other

[Ivshti]

@Picodes we tend to disagree with the severity here - gas attacks are possible in almost all cases of using Ambire accounts through a relayer. It's also an inherent design compromise of ERC-4337 as well, and the only way to counter it is with appropriate offchain checks/reputation systems and griefing protections.

Also, the solution seems too finicky. What if the tryCatch is called within execute (which it very likely will), which requires even more gas left to complete? Then 1) the solution won't be reliable 2) the attacker can make the attack anyway through execute

[Picodes]

The main issue here is that the nonce is incremented despite the fact that the transaction wasn't executed as intended, which would force the user to resign the payload and would be a griefing attack against the user. I do break an important invariant which is that if the nonce is incremented, the transaction signed by the user was included as he intended.

Also, I think this can be used within tryCatchLimit to pass a lower gasLimit: quoting EIP150:
"If a call asks for more gas than the maximum allowed amount (i.e. the total amount of gas remaining in the parent after subtracting the gas cost of the call and memory expansion), do not return an OOG error; instead, if a call asks for more gas than all but one 64th of the maximum allowed amount, call with all but one 64th of the maximum allowed amount of gas (this is equivalent to a version of EIP-901 plus EIP-1142)."

[c4-judge]

Picodes marked the issue as satisfactory

[Ivshti]

The main issue here is that the nonce is incremented despite the fact that the transaction wasn't executed as intended, which would force the user to resign the payload and would be a griefing attack against the user. I do break an important invariant which is that if the nonce is incremented, the transaction signed by the user was included as he intended.

Also, I think this can be used within tryCatchLimit to pass a lower gasLimit: quoting EIP150: "If a call asks for more gas than the maximum allowed amount (i.e. the total amount of gas remaining in the parent after subtracting the gas cost of the call and memory expansion), do not return an OOG error; instead, if a call asks for more gas than all but one 64th of the maximum allowed amount, call with all but one 64th of the maximum allowed amount of gas (this is equivalent to a version of EIP-901 plus EIP-1142)."

I'm not sure I undestand, the whole point of signing something that calls into tryCatch is that you don't care about the case where the nonce is incremented but the transaction is failing. What am I missing?

[Picodes]

the whole point of signing something that calls into tryCatch is that you don't care about the case where the nonce is incremented but the transaction is failing

You don't care if the transactions fail because the sub-call is invalid, but you do if it's because the relayer manipulated the gas, right?

[Ivshti]

the whole point of signing something that calls into tryCatch is that you don't care about the case where the nonce is incremented but the transaction is failing

You don't care if the transactions fail because the sub-call is invalid, but you do if it's because the relayer manipulated the gas, right?

Ok, I see the point here - prob repeating stuff that others said before but, trying to simplify - the relayer can rug users by taking their fee regardless of the fact that the inner transactions fail due to the relayer using a lower gasLimit. This would be possible if some of the sub-transactions use tryCatch but the fee payment does not.

However, I'm not sure how the mitigation would work. Can't the relayer still calculate a "right" gas limit for which the tryCatch will fail but the rest will succeed?

[Picodes]

My understanding is that as usinggasleft() > gasBefore/64, we know for sure than the inner call didn't failed due to an out of gas as it was called with 63*gasBefore/64. So the relayer has to give enough gas for every subcall to execute fully, whether it is successful or not.

[Ivshti]

I see, this sounds reasonable, i need a bit more time eto think about it and if it is, we'll apply this mitigation