c4-submissions commented 1 year ago

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:

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 commented 1 year ago

0xA5DF marked the issue as duplicate of #858

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)

alcueca commented 1 year ago

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