Maia DAO - Ulysses - Audinarey's results

0xA5DF marked the issue as primary issue

0xA5DF marked the issue as sufficient quality report

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

0xBugsy (sponsor) disputed

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.

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.

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

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

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.

alcueca changed the severity to QA (Quality Assurance)

alcueca marked the issue as grade-a

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.

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.

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: https://github.com/code-423n4/2023-09-maia-findings/discussions/906#discussioncomment-7408115

0xLightt (sponsor) confirmed

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

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.