users may not be able to call retryDeposit(...) for failed deposits

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L882
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L832
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L354

Vulnerability details

Impact

In the event of a failed deposit the has not been executed yet, the user may not be able to call retryDeposit(...) if the user does not specify his account as the refundee.

Proof of Concept

When _createDeposit(...) is called as shown below, the deposit.owner variable is updated to _refundee.

    function _createDeposit(
        uint32 _depositNonce,
        address payable _refundee,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit
    ) internal {
        // Update Deposit Nonce
        depositNonce = _depositNonce + 1;

        ...

        // Save deposit to storage
        ...
        // @audit SUGGESTION change deposit.owner = _refundee; to deposit.owner = msg.sender;
        deposit.owner = _refundee;

    }

When retryDeposit(...) is called, as shownn below, if the user did not use his address as the refundee during deposit creation, his the call to retryDeposit(...) will revert.

    function retryDeposit(
        bool _isSigned,
        uint32 _depositNonce,
        bytes calldata _params,
        GasParams calldata _gParams,
        bool _hasFallbackToggled
    ) external payable override lock {
        ...

        //Check if deposit belongs to message sender

        if (deposit.owner != msg.sender) revert NotDepositOwner();

        ...

        // Perform Call
        _performCall(payable(msg.sender), payload, _gParams);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider making the msg.sender the owner during the deposir creation stage as shown below

    function _createDeposit(
        uint32 _depositNonce,
        address payable _refundee,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit
    ) internal {
        
        ...

        // Save deposit to storage
        ...
        deposit.owner = msg.sender;

    }

Assessed type

Invalid Validation

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[0xA5DF]

Seems like a design choice, but will leave open for sponsor to comment

[c4-sponsor]

0xBugsy (sponsor) disputed

[0xLightt]

This is intended, you can set the owner of the deposit to your own address if desired. Removing this would break functionality, like our current router interactions.

[alcueca]

It might be intended, but that is far from what the docs suggest.

It doesn't seem far-fetched to think that users might want to set _refundee to some address other than msg.sender. It doesn't make sense either that if the user decides to set the _refundee to something else than msg.sender then they lose some functionality unrelated to gas refunds.

[alcueca]

Maybe the deposit owner and the refundee should be separate concepts, and dealt with separately.

[0xBugsy]

We believe this relies on API misuse. Someone integrating with our contracts should be conscious of what's the role of a _refundee so as to prevent setting ownership over user deposits or settlements to other addresses.

The only use case we can see where having both addresses exposed, would be if someone were to create a router contract that is intended to retain ownership over user deposits (e.g. needs to perform some extra action before giving tokens back to user) and wanted to refund excess gas to the caller, this can also be achieved by other means without having to increase the number of parameters for everyone else.

In addition, for signed calls letting the user decide the _owner could lead to unexpected outcomes in our opinion since we would be using different Virtual Accounts in the first call and the second.

Regarding the natspec we 100% agree it is not up to par and should be updated to match the detail used in IRootBridgeAgent: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/interfaces/IRootBridgeAgent.sol#L178C1-L179C120

[alcueca]

I'll judge it as QA grade-a, but please fix the natspec and possibly the variable name. It is very misleading. I'd argue that the fact that the variable denotes the deposit owner is much more relevant than the fact that it also denotes the recipient for gas refunds.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[0xLightt]

As bugsy stated:

Someone integrating with our contracts should be conscious of what's the role of a _refundee so as to prevent setting ownership over user deposits or settlements to other addresses.

The _refundee is meant to be the owner, especially for non-signed calls that are router/contract facing functions. We should rename that variable and/or split it into two.

In addition, for signed calls letting the user decide the _owner could lead to unexpected outcomes in our opinion since we would be using different Virtual Accounts in the first call and the second.

For signed calls, the msg.sender should be the owner or what bugsy described would happen. We are not doing this part correctly, the _refundee is being used instead of msg.sender here for example: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L299C39-L299C48

I still believe this would only happen due to API misuse, a UI should always pass the caller's address as _refundee. And if a EOA is able to build proper callOutSignedAndBridge parameters, it is expected that they would also put their own or a trusted/owned address in _refundee. But in case anyone ever makes the mistake, to avoid any issues, we should only use the _refundee address for gas refunds or take out _refundee parameter completely for signed calls and use msg.sender for all purposes.

[viktorcortess]

As bugsy stated:

Someone integrating with our contracts should be conscious of what's the role of a _refundee so as to prevent setting ownership over user deposits or settlements to other addresses.

The _refundee is meant to be the owner, especially for non-signed calls that are router/contract facing functions. We should rename that variable and/or split it into two.

In addition, for signed calls letting the user decide the _owner could lead to unexpected outcomes in our opinion since we would be using different Virtual Accounts in the first call and the second.

For signed calls, the msg.sender should be the owner or what bugsy described would happen. We are not doing this part correctly, the _refundee is being used instead of msg.sender here for example: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L299C39-L299C48

I still believe this would only happen due to API misuse, a UI should always pass the caller's address as _refundee. And if a EOA is able to build proper callOutSignedAndBridge parameters, it is expected that they would also put their own or a trusted/owned address in _refundee. But in case anyone ever makes the mistake, to avoid any issues, we should only use the _refundee address for gas refunds or take out _refundee parameter completely for signed calls and use msg.sender for all purposes.

Hello 0xLightt , I have one of the duplicates of this issue and after reading your comments I'd like to add a comment.
In "real life" it's clear that the dApp and API will be set up correctly. But we audit the code with all possible interactions and the problem here is not in the names of the variables.

If we want to use a Branch side we can use ether callOutAndBridge() function from BaseBranchRouter contract or callOutAndBridge from BranchBridge contract(). Both of them don't have any modifier so a user can call any of them
and in case if he calls it from the BaseBranchRouter the refundee is indeed msg.sender:

//Perform call to bridge agent.
    IBridgeAgent(localBridgeAgentAddress).callOutAndBridge{value: msg.value}(
        payable(msg.sender), _params, _dParams, _gParams
    );

But if a user uses BranchBridge contract he can set up whatever he wants as a refundee and this can later lead to problems described in the issues linked with retry functions.

 function callOutAndBridge(
    address payable _refundee,
    bytes calldata _params,
    DepositInput memory _dParams,
    GasParams calldata _gParams
) external payable override lock {  

Even if it looks like a user error at first I noticed that similar Root functions are protected by modifiers, so the agent only can be called by a router, not by a user:

RootBridgeAgent:

175: function callOutAndBridge(
    address payable _refundee,
    address _recipient,
    uint16 _dstChainId,
    bytes calldata _params,
    SettlementInput calldata _sParams,
    GasParams calldata _gParams,
    bool _hasFallbackToggled
) external payable override lock requiresRouter {


/// @notice Internal function to verify msg sender is Bridge Agent's Router.
modifier requiresRouter() {
    if (msg.sender != localRouterAddress) revert UnrecognizedRouter();
    _;
}

I am leading to the point that maybe bridgeAgent contract should have the same modifier then the user won't be able to call bridge contract's call functions without router?
At the current implementation, the bridge has 2 entry points if a user wants to use call() functions. Maybe it's the designed behavior but I think these 2 possibilities to call the same function attracted the attention of the wardens and I personally assumed that it was a forgotten modifier, not just the name of a variable.

[0xLightt]

Your recommendation to add requiresRouter to every non-signed callOut in the branch and remove the system calls makes complete sense.

I believe the reason for QA is that due to happening from a user error, so it follows the first point: #906 (comment)

[c4-sponsor]

0xLightt (sponsor) confirmed

[viktorcortess]

Hello @alcueca, may I request your attention to the comments above, and I kindly ask for your final decision on this matter.

[alcueca]

Again, a user calling this function can only hurt themselves if they call it with the wrong parameters. To be consistent in judging this contest I'm classifying users hurting themselves as QA, unless it is explicitly said that the users should do something, and that something hurts them.

To be a High or a Medium in this contest:

• Users (or attackers) hurt other users, or
• Users hurt themselves by doing something they were told to do by the sponsor.

The sponsor modifying the code to remove footguns is consistent with a QA grade-a severity, as well.