Maia DAO Ecosystem - Noro's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L441

Vulnerability details

Impact

BranchBridgeAgent._sendRetrieveOrRetry() calls _createGasDeposit that calls _depositGas to transfer wrapped native token to the Port, but the wrapped native token is not deposited so it can be transferred, thus if the Port does not already have wrapped native token, the transfer will revert causing retrySettlement and retrieveDeposit calls to revert . User will actually be able to top up and retrieve his deposits not loosing any funds but he would spend double the needed gas and that is not intended .

Proof of Concept

1. the following test is testing the case of retrieveDeposit call, but retrySettlement will revert as well .
2. the following test can be run in BranchBridgeAgentTest.t.sol

function testRetrieveDepositRevert() public {
        vm.mockCall(
            localAnyCallExecutorAddress,
            abi.encodeWithSignature("context()"),
            abi.encode(rootBridgeAgentAddress, rootChainId, 22)
        );

        testCallOutWithDeposit();

        vm.deal(address(this), 1 ether);

        vm.expectRevert(abi.encodeWithSignature("TransferFailed()"));
        bAgent.retrieveDeposit{value: 0.5 ether}(1);
    }

Tools Used

Manual review

Recommended Mitigation Steps

deposit msg.value in wrappedNativeToken before calling _createGasDeposit

function _sendRetrieveOrRetry(bytes memory _data) internal {
        // //Wrap the gas allocated for omnichain execution.
        wrappedNativeToken.deposit{value: msg.value}();

        //Deposit Gas for call.
        _createGasDeposit(msg.sender, msg.value.toUint128());

        //Perform Call
        _performCall(_data);
    }

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1227

Vulnerability details

Impact

0x08 flag is a call by BranchBridgeAgent.retrieveDeposit() that clears a deposit that has not been executed yet triggering anyFallback, but doesn’t catch that call in its if else structure , thus retrieveDeposit loses its functionality, but user still be able to call retryDeposit with enough gas to fail a call fallback wasting greater gas and fortunately no funds are bricked inside contract .

Proof of Concept

the following test can be run in BranchBridgeAgentTest.t.sol

function testAnyFallbackDoNotCatchFlag() public {
        vm.mockCall(
            localAnyCallExecutorAddress,
            abi.encodeWithSignature("context()"),
            abi.encode(rootBridgeAgentAddress, rootChainId, 22)
        );

        //Get some gas.
        vm.deal(address(this), 1 ether);

        //Mint Test tokens.
        underlyingToken.mint(address(this), 100 ether);

        //Approve spend by router
        underlyingToken.approve(localPortAddress, 100 ether);

        //Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(testToken),
            token: address(underlyingToken),
            amount: 100 ether,
            deposit: 100 ether,
            toChain: rootChainId
        });

        //Call Deposit function
        IBranchRouter(bRouter).callOutAndBridge{value: 1 ether}(bytes("test"), depositInput, 0.001 ether);

        vm.deal(localPortAddress, 1 ether);

        //Prepare deposit info
        DepositParams memory depositParams = DepositParams({
            hToken: address(testToken),
            token: address(underlyingToken),
            amount: 100 ether,
            deposit: 100 ether,
            toChain: rootChainId,
            depositNonce: 1,
            depositedGas: 1 ether
        });

        // Encode AnyFallback message
        bytes memory anyFallbackData = abi.encodePacked(
            bytes1(0x08),
            depositParams.depositNonce,
            depositParams.hToken,
            depositParams.token,
            depositParams.amount,
            depositParams.deposit,
            depositParams.toChain,
            bytes("testdata"),
            depositParams.depositedGas,
            depositParams.depositedGas / 2
        );

        vm.mockCall(
            address(localAnyCongfig),
            abi.encodeWithSignature(
                "calcSrcFees(address,uint256,uint256)", address(0), rootChainId, anyFallbackData.length
            ),
            abi.encode(0)
        );

        // deposit native token to prevent transfer failing on retrieveDeposit() call (another issue) .
        vm.deal(address(bAgent), 0.5 ether);
        vm.prank(address(bAgent));
        WETH9(wrappedNativeToken).deposit{value: 0.5 ether}();

        //Call retrieveDeposit
        vm.deal(address(this), 1 ether);
        bAgent.retrieveDeposit{value: 0.5 ether}(1);

        // Call 'anyFallback'
        vm.prank(localAnyCallExecutorAddress);
        bAgent.anyFallback(anyFallbackData);

        Deposit memory deposit = bAgent.getDepositEntry(1);

        require(deposit.status == DepositStatus.Success); // deposit status is still on success .
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add the flag 0x08, to the first if statement condition in the function . if ((flag == 0x00) || (flag == 0x01) || (flag == 0x02) || (flag == 0x08))

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L173 https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L179

Vulnerability details

Impact

variation from the standard could break composability and potentially lead to loss of funds

Proof of Concept

EIP-4626 specifies concerning maxWithdraw :

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

and maxRedeem :

MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

The implementation does not conform to this specification. Except for first Tuesday of each month, withdraws and redeems are disabled. In such a case, these functions MUST return 0.

Tools Used

Manual review

Recommended Mitigation Steps

check when the withdraw and redeems are disabled and return 0 if so .

Assessed type

ERC4626