Redeeming a Settlement won't work for unsigned messages when the communicating dApps have different addresses on the different chains

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L163-L171
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L186-L194
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L311-L315

Vulnerability details

Impact

Funds cannot be redeemed and remain stuck in a settlement

Proof Of Concept

In MulticallRootRouter execute() calls _approveAndCallOut(...), however, it passes the Output Params recipient also as the refundee. This is dangerous because the recipient Dapp on the BranchChain can have a different address or not exist on the Root Chain and therefore if a settlement fails it won't be able to be redeemed since the settlement owner is set as the refundee. Here is a scenario -

1. dApp A on a Branch Chain with (address = 0xbeef) initiates a CallOut(...) 0x01 with OutputParams (0x01) for the RootRouter
2. RootBridgeAgent executor calls MulticallRootRouter execute() which then performs some number of arbitrary calls and gets the OutputParams assets into the MulticallRootRouter
3. MulticallRootRouter attempts to bridge out the assets to the BranchChain and creates a settlement, passing the recipient (address = 0xbeef) but also sets the refundee as (address = 0xbeef).
4. If the settlement fails there is no guarantee that 0xbeef is a known dApp on the Root Chain and the assets won't be able to be redeemed.

function execute(bytes calldata encodedData, uint16) external payable override lock requiresExecutor {
        // Parse funcId
        bytes1 funcId = encodedData[0];
				
				// code ...
            /// FUNC ID: 2 (multicallSingleOutput)
        } else if (funcId == 0x02) {
            // Decode Params
            (
                IMulticall.Call[] memory callData,
                OutputParams memory outputParams,
                uint16 dstChainId,
                GasParams memory gasParams
            ) = abi.decode(_decode(encodedData[1:]), (IMulticall.Call[], OutputParams, uint16, GasParams));

            // Perform Calls
            _multicall(callData);

            // Bridge Out assets
            _approveAndCallOut(
                outputParams.recipient,
                outputParams.recipient,
                outputParams.outputToken,
                outputParams.amountOut,
                outputParams.depositOut,
                dstChainId,
                gasParams
            );
	
				}
// code ...
    }

function _createSettlement(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        bytes memory _params,
        address _globalAddress,
        uint256 _amount,
        uint256 _deposit,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
        // code ...

        // Update Setttlement
        settlement.owner = _refundee;
        settlement.recipient = _recipient;

				// code ...
      
    }

function redeemSettlement(uint32 _settlementNonce) external override lock {
        // Get setttlement storage reference
        Settlement storage settlement = getSettlement[_settlementNonce];

        // Get deposit owner.
        address settlementOwner = settlement.owner;

        // Check if Settlement is redeemable.
        if (settlement.status == STATUS_SUCCESS) revert SettlementRedeemUnavailable();
        if (settlementOwner == address(0)) revert SettlementRedeemUnavailable();

        // Check if Settlement Owner is msg.sender or msg.sender is the virtual account of the settlement owner.
        if (msg.sender != settlementOwner) {
            if (msg.sender != address(IPort(localPortAddress).getUserAccount(settlementOwner))) {
                revert NotSettlementOwner();
            }
        }
			/// more code ...
    }

Tools Used

Manual Inspection

Recommended Mitigation Steps

Include an argument that enables users to specify the refundee when creating settlements without using a Virtual Account

Assessed type

Context

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-sponsor]

0xLightt (sponsor) confirmed

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked issue #351 as primary and marked this issue as a duplicate of 351

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as not a duplicate

[c4-judge]

alcueca marked the issue as primary issue

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

In this case, I'm going to mark this report as best despite #351 having a wider scope and a coded PoC, because this issue correctly shows a more serious impact. The sponsor should also look at #679 and remediate its findings.

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[stalinMacias]

Hello @alcueca

I'd like to bring this to the table. So, this report is about problems with redeeming settlements for unsigned messages. So, first and foremost, for redemption to be possible, a deposit should've been made in the first place, right? No deposit, no redemption. So, following this logic, and based on your comment on issue #685 , if users doing unsigned deposits is judged as qa, shouldn't the same criteria be applied to unsigned redemptions?

[alexxander77]

Hello again @stalinMacias,
The prerequisite to creating a settlement is a successful deposit (with correct instruction flag). The issue concerns integration with dapps which is the more likely target group and the sponsor recommendation is that they go through the unsigned path. Different to normal users (ie eoa's), smart contracts hold state that constantly changes & the reason a settlement can fail is manifold (not enough liquidity, upgrading, pausing, malformed message, etc...), also keep in mind that cross-chain interactions are not atomic. That's why I believe sponsors have included the redemption mechanism which could brick arbitrary amount of funds because of a dangerous assumption that is done on creation (which the dapp/user doesn't have control over).

[stalinMacias]

@alexxander77 I see what you are saying, so, this issue is similar to #877 in the sense that the problem is for contract accounts that interact with the protocol. Here, the problem is for unsigned messages that don't involve the virtual account, but the problem is similar about the owner of the contract account may not own the same address in a different chain

Good catch

[0xBugsy]

Addressed at Maia-DAO/2023-09-maia-remediations@b429742