search

code-423n4 / 2023-05-maia-findings

24 stars 13 forks source link

Nonces of failed settlements is set to true, which prevents reopening of failed settlements #684

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1158 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1182 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1206

Vulnerability details

Impact

If for whatever reason, any of the calls made to BranchBridgeAgentExecutor from BranchBridgeAgent#anyExecute fails or reverts, the error is caught in a try-catch, and the executionHistory for that settlementNonce is updated to true. The result is that calling retrySettlement with that failed settlementNonce as parameter would never succeed because BranchBridgeAgent checks the executionHistory, and when it sees that the settlementNonce is "true", it assumes that it was successful.

Proof of Concept

For example, here is the logic for 0x01 flag in BranchBridgeAgent#anyExecute function:

else if (flag == 0x01) {
    //Get Settlement Nonce
    uint32 nonce = uint32(bytes4(data[PARAMS_START_SIGNED:25]));

    //Check if tx has already been executed
    if (executionHistory[nonce]) {
        _forceRevert();
        //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
        return (true, "already executed tx");
    }

    //Try to execute remote request
    try BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(
        recipient, localRouterAddress, data
    ) returns (bool, bytes memory res) {
        (success, result) = (true, res);
    } catch (bytes memory reason) {
        result = reason;
    }

    //Update tx state as executed
    executionHistory[nonce] = true;

    //DEPOSIT FLAG: 2 (Multiple Settlement)
}

In the try block, BranchBridgeAgentExecutor#executeWithSettlement is called, and if anything causes it to fail, the whole call will not revert, but (success,result) return values are set. After the errors have been caught, the catch block does not use the return keyword to exit the anyExecute call. This allows the other parts of the function to be called. As we can see, after try-catch block, executionHistory for that settlementNonce is updated to true. So, even for failed settlements(settlements that reverted midway and were not executed), the settlementNonce would be updated to true. Now, users will not be able to retrySettlement on the failed settlementNonce.

Occurences

I used 0x01 flag(which is for single asset settlement) as an example. Here are other occurrences of the bug:

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1158 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1182 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1206

Tools Used

Manual Review

Recommended Mitigation Steps

In the catch block of each try-catch in BranchBridgeAgent#anyExecute, instead of simply setting the result value to reason, always return (false, reason) to avoid updating executionHistory for that settlementNonce

Assessed type

Error

c4-judge commented 1 year ago

trust1995 marked the issue as primary issue

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor disputed

0xBugsy commented 1 year ago

It is intended, after reverting and catching success is left as false to trigger fallback and reopen settlement for redemption on root (otherwise routers could return false and double spend assets)

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

0xBugsy marked the issue as disagree with severity

0xBugsy commented 1 year ago

To give further context on why this is intended if anyFallback is triggered (success set to false) we must set the nonce as executed so as to prevent allowing double spending of assets via calling retrySettlement before fallback arrives to the root environment and afterwards calling redeemSettlement. The function retrySettlement only allows for bumping gas in case of outofgas and fallback not triggered

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor disputed

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid

Emedudu commented 1 year ago

Hi @0xBugsy , If a settlement fails on a Branch chain, a "false" success value is returned, which allows Multichain to call anyFallback in RootBridgeAgent to reopen that settlement. Once that settlement is reopened on RootBridgeAgent, user can call RootBridgeAgent#retrySettlement on that failed settlementNonce to try that settlement again. But the PROBLEM is, an attempt to retry that failed settlement will fail on BranchBridgeAgent because the executionHistory for the failed settlement has been set to true. Please take a look and read the //audit comment:

    //Try to execute remote request
    try BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(
        recipient, localRouterAddress, data
    ) returns (bool, bytes memory res) {
        (success, result) = (true, res);
    } catch (bytes memory reason) {
        result = reason;//@audit function call should `return` here to avoid setting executionHistory to true, which would prevent retrying of the failed settlement.
    }

    //Update tx state as executed
    executionHistory[nonce] = true;

But with this fix:

    try BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(
        recipient, localRouterAddress, data
    ) returns (bool, bytes memory res) {
        (success, result) = (true, res);
    } catch (bytes memory reason) {
@>    return (false, reason);
    }

    //Update tx state as executed
    executionHistory[nonce] = true;

If an error is caught, anyFallback will be triggered in RootBridgeAgent which would reopen settlement, and retrying of that settlement will work as expected on BranchBridgeAgent. Also have a look at issue 686

0xBugsy commented 1 year ago

Hi @0xBugsy , If a settlement fails on a Branch chain, a "false" success value is returned, which allows Multichain to call anyFallback in RootBridgeAgent to reopen that settlement. Once that settlement is reopened on RootBridgeAgent, user can call RootBridgeAgent#retrySettlement on that failed settlementNonce to try that settlement again. But the PROBLEM is, an attempt to retry that failed settlement will fail on BranchBridgeAgent because the executionHistory for the failed settlement has been set to true. Please take a look and read the //audit comment:

    //Try to execute remote request
    try BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(
        recipient, localRouterAddress, data
    ) returns (bool, bytes memory res) {
        (success, result) = (true, res);
    } catch (bytes memory reason) {
        result = reason;//@audit function call should `return` here to avoid setting executionHistory to true, which would prevent retrying of the failed settlement.
    }

    //Update tx state as executed
    executionHistory[nonce] = true;

But with this fix:

    try BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(
        recipient, localRouterAddress, data
    ) returns (bool, bytes memory res) {
        (success, result) = (true, res);
    } catch (bytes memory reason) {
@>    return (false, reason);
    }

    //Update tx state as executed
    executionHistory[nonce] = true;

If an error is caught, anyFallback will be triggered in RootBridgeAgent which would reopen settlement, and retrying of that settlement will work as expected on BranchBridgeAgent. Also have a look at issue 686

This is necessary otherwise we would introduce a race condition where you deploy a tx you know will revert leaving your deposit redeemable and another tx to retry said deposit one right after the other allowing you to double spend. Retrying is not allowed after redemption being triggered to avoid this!

Emedudu commented 1 year ago

@0xBugsy This would mean that retrySettlement function is of no use as it will always revert on BranchBridgeAgent

0xBugsy commented 1 year ago

@0xBugsy This would mean that retrySettlement function is of no use as it will always revert on BranchBridgeAgent

At minimum protects users against gas price unpredictability that may trigger a _forceRevert() without the need to wait and added cost for cross-chain execution of retrieveDeposit, calling locally redeemDeposit and only after creating a new callOutAndBridge

Emedudu commented 1 year ago

Please consider the following scenario:

If you consider the recommendation however,

0xBugsy commented 1 year ago

Please consider the following scenario:

  • settlement fails on branchbridgeagent, success value of false is returned
  • anyFallback is triggered on rootbridgeagent, which reopens the settlement
  • Now, user should be able to retry the failed settlement
  • If user tries to retrySettlement, the call will fail on branchbridgeagent because executionHistory for that settlementNonce has been set to true.
  • This means that retrySettlement is not usable

If you consider the recommendation however,

  • anyExecute call is returned from before executionHistory for that settlementNonce is set to true
  • user can now successfully retrySettlement because executionHistory for that settlementNonce has not been set to true(it has not been executed because the settlement failed)

I believe you are confusing retry and retrieve after a nonce is executed and set to true in executionHistory you can only redeem. Retry would work for a nonce not yet executed.

Consequences of remediation suggested are detailed here: https://github.com/code-423n4/2023-05-maia-findings/issues/684#issuecomment-1640906641

Emedudu commented 1 year ago

retrySettlement should be able to try again a settlement that failed on the BranchBridgeAgent but currently, all failed settlement's executionHistory is set to true on BranchBridgeAgent, which will prevent retrying of that settlement.

As for the "Consequences of remediation",

To give further context on why this is intended if anyFallback is triggered (success set to false) we must set the nonce as executed so as to prevent allowing double spending of assets via calling retrySettlement before fallback arrives to the root environment and afterwards calling redeemSettlement.

Nobody can call redeemSettlement immediately after calling retrySettlement because retrySettlement sets the settlement status to "Success", and redeemDeposit requires that it is "Failed". Also, retrySettlement cannot be called after redeemSettlement because redeemSettlement deletes that settlementNonce data

So the proposed recommendation will not open risks of double spending.

0xBugsy commented 1 year ago

Nobody can call redeemSettlement immediately after calling retrySettlement because retrySettlement sets the settlement status to "Success", and redeemDeposit requires that it is "Failed".

This is explained here:

To give further context on why this is intended if anyFallback is triggered (success set to false) we must set the nonce as executed so as to prevent allowing double spending of assets via calling retrySettlement before fallback arrives to the root environment and afterwards calling redeemSettlement.

More specifically:

via calling retrySettlement before fallback arrives

Emedudu commented 1 year ago

via calling retrySettlement before fallback arrives

The problem here is that ALL failed settlements on a Branch chain can NEVER be retried. If the settlement succeeded on branch chain, the retrySettlement call will fail(working as intended) But if the settlement failed on branch chain, the retrySettlement will still fail. That's the issue.

0xBugsy commented 1 year ago

via calling retrySettlement before fallback arrives

The problem here is that ALL failed settlements on a Branch chain can NEVER be retried. If the settlement succeeded on branch chain, the retrySettlement call will fail(working as intended) But if the settlement failed on branch chain, the retrySettlement will still fail. That's the issue.

This is invalid. If gas management is successful and fallback is executed that is already an execution, if we left it to false like you are suggesting we would be open to double spending since we cant have a tx be redeemable and retry-able in two completely different networks at the same time