Maia DAO - Ulysses - mert_eren's results

Lines of code

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

Vulnerability details

Impact

VirtualAccounts are created for user specific and it holds user's financial assets. VirtualAccount contract has call and payableCall functions purpose is make this function like a EOA. So this is important that there should be important put access control for this call functions. If there is not, any malicious user can use this call function to steal asset's of virtualAccount's user. As can be seen call function has access control which can be seen as requiresApprovedCaller modifier.

    function call(Call[] calldata calls) external override requiresApprovedCaller returns (bytes[] memory returnData) {

    modifier requiresApprovedCaller() {
        if (!IRootPort(localPortAddress).isRouterApproved(this, msg.sender)) {
            if (msg.sender != userAddress) {
                revert UnauthorizedCaller();
            }
        }
        _;
    }

However there is no such that modifier in payableCall or any other access control so anybody can call payableCall .

    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }


            bool success;


            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);


            if (!success) revert CallFailed();


            unchecked {
                ++i;
            }
        }


        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

It can be seen that there is no access control for msg.sender. Due to this lack,for example, a malicious user can give parameter as target USDT and calldata as transfer(addressOfMalUser,BalanceOfVirtualAccount). So by this way malicious user can steal users assets without revert.

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L490-L510 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L82-L106 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L351-L384 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L277-L291

Vulnerability details

Impact

If user call callOutAndBridge function, _params parameter encoded with deposit parameter in payload and it is used in RootBridgeAgentExecutor.executeWithDeposit function.

    function callOutAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {
        //Cache Deposit Nonce
        uint32 _depositNonce = depositNonce;


        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(
            bytes1(0x02), _depositNonce, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _params
        );


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


        //Perform Call
        _performCall(_refundee, payload, _gParams);
    }

This function will send message to layerzero and after that RootBridgeAgent called with lzReceive. lzReceive call RootBridgeAgentExecutor.executeWithDeposit.

    function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        // Read Deposit Params
        DepositParams memory dParams = DepositParams({
            depositNonce: uint32(bytes4(_payload[PARAMS_START:PARAMS_TKN_START])),
            hToken: address(uint160(bytes20(_payload[PARAMS_TKN_START:PARAMS_TKN_START_SIGNED]))),
            token: address(uint160(bytes20(_payload[PARAMS_TKN_START_SIGNED:45]))),
            amount: uint256(bytes32(_payload[45:77])),
            deposit: uint256(bytes32(_payload[77:PARAMS_TKN_SET_SIZE]))
        });


        // Bridge In Assets
        _bridgeIn(_router, dParams, _srcChainId);


        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

It can be seen that in payload, after PARAMS_TKN_SET_SIZE bytes correspond to _params in callOutAndBridge.And if _params is empty this function will not call router.executeDepositSingle. So executeWithDeposit function just run _bridgeIn function.

    function _bridgeIn(address _recipient, DepositParams memory _dParams, uint16 _srcChainId) internal {
        //Request assets for decoded request.
        RootBridgeAgent(payable(msg.sender)).bridgeIn(_recipient, _dParams, _srcChainId);
    }

This function call RootBridgeAgent.bridgeIn.

    function bridgeIn(address _recipient, DepositParams memory _dParams, uint256 _srcChainId)
        public
        override
        requiresAgentExecutor
    {
        // Deposit can't be greater than amount.
        if (_dParams.amount < _dParams.deposit) revert InvalidInputParams();


        // Cache local port address
        address _localPortAddress = localPortAddress;


        // Check local exists.
        if (_dParams.amount > 0) {
            if (!IPort(_localPortAddress).isLocalToken(_dParams.hToken, _srcChainId)) {
                revert InvalidInputParams();
            }
        }


        // Check underlying exists.
        if (_dParams.deposit > 0) {
            if (IPort(_localPortAddress).getLocalTokenFromUnderlying(_dParams.token, _srcChainId) != _dParams.hToken) {
                revert InvalidInputParams();
            }
        }


        // Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
        IPort(_localPortAddress).bridgeToRoot(
            _recipient,
            IPort(_localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _srcChainId),
            _dParams.amount,
            _dParams.deposit,
            _srcChainId
        );
    }

And this will call rootPort.bridgeToRoot to mint globalHtoken to recepient.,

    function bridgeToRoot(address _recipient, address _hToken, uint256 _amount, uint256 _deposit, uint256 _srcChainId)
        external
        override
        requiresBridgeAgent
    {
        if (!isGlobalAddress[_hToken]) revert UnrecognizedToken();


        if (_amount - _deposit > 0) {
            unchecked {
                _hToken.safeTransfer(_recipient, _amount - _deposit);
            }
        }


        if (_deposit > 0) if (!ERC20hTokenRoot(_hToken).mint(_recipient, _deposit, _srcChainId)) revert UnableToMint();
    }

The recipient parameter come from executeWithDeposit function and it can be seen that it indicate router.So RootRouter take globalHTokens.In short callOutAndBridge take asset from user in branch token and mint globalhToken in root chain rooRouter and transaction finish without revert. This globalHTokens cannot be taken by user after that and this tokens cannot be tranfered out of the router. Moreover due to transaction finish successfully user cannot take his underlying token with retriveDeposit because retriveDeposit can be work only if transaction in the root chain reverted. In short user lose his underlyingtoken and his tokens will stuck in RootRouter permanently.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Assessed type

Invalid Validation