Deposit owner is incorrectly set to _address to pay refunds to_ when depositing via BranchBridgeAgent instead of caller or this behavior is not specified

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/IBranchBridgeAgent.sol#L154-L168
https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/IBranchBridgeAgent.sol#L170-L184
https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/IBranchBridgeAgent.sol#L186-L196
https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/IBranchBridgeAgent.sol#L198-L214
https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/IBranchBridgeAgent.sol#L216-L233

Vulnerability details

Summary

When depositing assets (single, multiple or signed/unsigned) to the Ulysses Omnichain system via BranchBridgeAgents, in all deposit related functions, user is instructed to provide an address where excess gas from msg.value will be deposited to. Example for IBranchBridgeAgent::callOutAndBridge):

     * @notice Function to perform a call to the Root Omnichain Router while depositing a single asset.
     *   @param gasRefundee address to return excess gas deposited in `msg.value` to.

and for all other functions:

• callOutAndBridge
• callOutAndBridgeMultiple
• callOutSigned
• callOutSignedAndBridge
• callOutSignedAndBridgeMultiple

But in all these implementation, the deposit owner is actually set to this provided refund address, not the message sender,

        //Create Deposit and Send Cross-Chain request
        _createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

while tokens are taken from the message sender.

        // Deposit / Lock Tokens into Port
        IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);

If this behavior is intentional it must be documented as such since users of the protocol may end up giving ownership to addresses they did not intend to thus losing their deposits.

Impact

Users may incorrectly give ownership of their deposits to unwanted addresses that may lead to user funds being lost.

Tools Used

Manual review

Note for judging

This issue was confirmed by sponsors as being a documentation flaw in private. For the IRootBridgeAgent, for example, the full role of the address parameter was mentioned. However, as it is, the system user-interaction should be considered flawed for the Branch Agent and fixed as such.

Recommendations

Either document this behavior, similar to how in IRootBridgeAgent it was mentioned, or modify that the owner to be set to the message sender and also document this accordingly.

Assessed type

Other

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #858

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[alcueca]

Documentation issue, the _refundee is actually the deposit owner if the caller decides that it should be anyone else.

[c4-judge]

alcueca marked the issue as grade-a