c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchBridgeAgent.sol#L89

Vulnerability details

Impact

Users can lose native tokens if they call ArbitrumBranchBridgeAgent.sol:retrySettlement() with msg.value > 0.

Proof of Concept

Since ArbitrumBranchBridgeAgent is an branch bridge agent deployed on the arbitrum chain, we override the retrySettlement() function to let users retry settlements through the root agent directly. However, when override to forbid this function, we make this function always successful:

    /// @inheritdoc IBranchBridgeAgent
    /// @dev This functionality should be accessed from Root environment
    function retrySettlement(uint32, bytes calldata, GasParams[2] calldata, bool) external payable override lock {}

Also, we only rewrite the @dev natspec tag, which is for "Explain to a developer any extra details", so the @notice tag, which is for "Explain to an end user what this does" is unchanged.

This can lead:

User A calls ArbitrumBranchBridgeAgent.sol:retrySettlement() with msg.value > 0, the tx is successful and msg.value is left in the bridge agent.
The bridge agent received a message and trigger executeNoSettlement, executeWithSettlement, etc. In which the balance of the agent (includes User A's in step 1) will be transferred out to router and used later:

    function _execute(uint256 _settlementNonce, bytes memory _calldata) private {
        //Update tx state as executed
        executionState[_settlementNonce] = STATUS_DONE;

        //Try to execute the remote request
        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

User A didn't achieve the retry and lost his native tokens.

Tools Used

Manual Review.

Recommended Mitigation Steps

Just like executeDepositSingle, executeDepositMultiple, executeSigned, executeSignedDepositMultiple in the CoreRootRouter.sol, we should directly revert.
Change the @notice natspec tag also.

Assessed type

ETH-Transfer

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

0xA5DF commented 1 year ago

Seems like a user error, but maybe there's a case for an exception here since it's a mistake that's easy for users to fall for. Leaving open for sponsor to comment

c4-sponsor commented 1 year ago

0xBugsy (sponsor) confirmed

c4-sponsor commented 1 year ago

0xBugsy marked the issue as disagree with severity

0xBugsy commented 1 year ago

No settlements will ever be retriable from Arbitrum Branch, so this is highly unlikely to happen in production. But we will address this.

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)