retryDeposit() doesn’t function to specification

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L319-L399
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174

Vulnerability details

Impact

Ulysses-omnichain retryDeposit(...) in BranchBridgeAgent.sol doesn’t function properly.

Proof of Concept

The idea behind retryDeposit(...) is to try and re-send a message that could have failed on the Root Chain, moreover, it is specific to messages that carry token information to perform token bridging. retryDeposit(...) reuses the same deposit nonce and re-sends the message to Root with that deposit nonce. A message could have failed for a variety of reasons when anyExecute(...) is called on the RootBridgeAgent - this could be gas issues, faulty router integrations or incorrectly formed extra data for further execution (we can also pass new data to retryDeposit(...) ). The problem is that in some cases, if a message fails executionHistory[fromChainId][nonce] = true- meaning that if we try and use the retryDeposit functionality the nonce will have already been used and we won’t be executing anything. In the code snippet below we can see that the try{} statement will catch any reverts that could be due to non-existing token addresses, router integrations with extra data supplied etc. , however it will still set executionHistory to true for that nonce value, therefore, if we were to retryDeposit(...) it will fail because it was already executed, therefore, retryDeposit(...) functionality is rendered useless in that case.

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

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

            //Get User Virtual Account
            VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                address(uint160(bytes20(data[PARAMS_START:PARAMS_START_SIGNED])))
            );

            //Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            //Try to execute remote request
            try RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDepositMultiple(
                address(userAccount), localRouterAddress, data, fromChainId
            ) returns (bool, bytes memory res) {
                (success, result) = (true, res);
            } catch (bytes memory reason) {
                result = reason;
            }

            //Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

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

Tools used

Manual inspection

Recommendation

Rework retryDeposit to supply a new nonce, or consider to leave executionHistory[fromChainId][nonce] = false for some reverts.

Assessed type

Context

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-sponsor]

0xBugsy marked the issue as sponsor confirmed

[0xBugsy]

Changing nonce of same deposit info would open doors to race conditions through use of retrieve and retry, ideal approach is to leave nonces that triggered a fallback as unexecuted so its not only useful in out of gas conditions

[c4-judge]

trust1995 marked the issue as selected for report

[c4-sponsor]

0xBugsy marked the issue as sponsor disputed

[0xBugsy]

Similar consequence to #684 but different approach by updating nonces

A deposit which has triggered a fallback and is potentially redeemable in a Branch chain cannot be left as unexecuted since it has in fact been executed otherwise this would allow for double spending in root and branch.

[c4-sponsor]

0xBugsy requested judge review

[Emedudu]

This issue and issue 686 are similar
It explains how the retryDeposit function is completely unusable.
Please check issue 686 for more context(especially the recommendation)

[0xBugsy]

This issue and issue 686 are similar It explains how the retryDeposit function is completely unusable. Please check issue 686 for more context(especially the recommendation)

Functioning of retryDeposit is detailed in natspec comments so it is in fact functioning to specification / as intended https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IBranchBridgeAgent.sol#L232

[Emedudu]

retryDeposit, according to the natspec is used to retry a failed deposit that hasn't been executed yet. But the problem is that all failed deposits' execution history is set to true, so it won't be possible to retry those failed deposits because even though they have not been executed, their executionHistory is set to true:

            //Try to execute remote request
            try RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDepositMultiple(
                address(userAccount), localRouterAddress, data, fromChainId
            ) returns (bool, bytes memory res) {
                (success, result) = (true, res);
            } catch (bytes memory reason) {
                result = reason;
            }

            //Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

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

If a deposit fails, it is caught in try-catch block, but function is not exited so executionHistory for that deposit which failed is set to true.
An attempt to retryDeposit on that failed deposit will never succeed because RootBridgeAgent checks the executionHistory for that depositNonce, and when it sees that it is "true", it _forceReverts

In the catch block, the function should be exited using the "return" keyword to avoid setting the executionHistory for a failed deposit to true(because that would prevent the deposit from being retried).

[0xBugsy]

retryDeposit, according to the natspec is used to retry a failed deposit that hasn't been executed yet. But the problem is that all failed deposits' execution history is set to true, so it won't be possible to retry those failed deposits because even though they have not been executed, their executionHistory is set to true:

            //Try to execute remote request
            try RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDepositMultiple(
                address(userAccount), localRouterAddress, data, fromChainId
            ) returns (bool, bytes memory res) {
                (success, result) = (true, res);
            } catch (bytes memory reason) {
                result = reason;
            }

            //Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

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

If a deposit fails, it is caught in try-catch block, but function is not exited so executionHistory for that deposit which failed is set to true. An attempt to retryDeposit on that failed deposit will never succeed because RootBridgeAgent checks the executionHistory for that depositNonce, and when it sees that it is "true", it _forceReverts

In the catch block, the function should be exited using the "return" keyword to avoid setting the executionHistory for a failed deposit to true(because that would prevent the deposit from being retried).

If executionHistory is set to true this means the deposit has been executed and in the scenario you described previously is available for redemption in the origin branch chain and cannot be executed again so as to avoid double spending since we can't distinguish an execution that ended successfully from one that ended up in fallback.

For the feature you are suggesting we could have 3 states instead of 2 which is covered in the following unrelated issue #183 .

[Emedudu]

If executionHistory is set to true this means the deposit has been executed

This is how it should be.
If a deposit has been executed, its executionHistory should be set to true, but if it failed, its executionHistory should remain as false, to allow a retry.

But now, if a deposit fails, the error that caused the failure is caught in a try-catch, and the remaining parts of the anyExecute function still gets executed(meaning that executionHistory for that failed deposit will WRONGLY be set to true):

            //DEPOSIT FLAG: 2 (Call with Deposit)
        } else if (flag == 0x02) {
            //Get Deposit Nonce
            uint32 nonce = uint32(bytes4(data[PARAMS_START:PARAMS_TKN_START]));

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

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

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

If RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, data, fromChainId) fails(i.e. deposit failed), error is caught in a try-catch, and after that, this is done:

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

So deposits that failed within the RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, data, fromChainId) call, are wrongly set to true in the executionHistory.

The recommendation is that within the catch block(which will only be entered if a deposit fails), the function should be exited with a return value of false. This will prevent executionHistory for that FAILED deposit to be set to true, allowing retry of a deposit that failed.

[0xBugsy]

If executionHistory is set to true this means the deposit has been executed

This is how it should be. If a deposit has been executed, its executionHistory should be set to true, but if it failed, its executionHistory should remain as false, to allow a retry.

But now, if a deposit fails, the error that caused the failure is caught in a try-catch, and the remaining parts of the anyExecute function still gets executed(meaning that executionHistory for that failed deposit will WRONGLY be set to true):

            //DEPOSIT FLAG: 2 (Call with Deposit)
        } else if (flag == 0x02) {
            //Get Deposit Nonce
            uint32 nonce = uint32(bytes4(data[PARAMS_START:PARAMS_TKN_START]));

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

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

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

If RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, data, fromChainId) fails(i.e. deposit failed), error is caught in a try-catch, and after that, this is done:

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

So deposits that failed within the RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, data, fromChainId) call, are wrongly set to true in the executionHistory.

The recommendation is that within the catch block(which will only be entered if a deposit fails), the function should be exited with a return value of false. This will prevent executionHistory for that FAILED deposit to be set to true, allowing retry of a deposit that failed.

This is invalid. If gas management is successful and fallback is executed that is already an execution, if we left it to false 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

[c4-judge]

trust1995 marked the issue as not selected for report

[c4-judge]

trust1995 marked the issue as unsatisfactory:
Invalid