Maia DAO Ecosystem - zzebra83's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/BranchBridgeAgent.sol#L554-L560

Vulnerability details

Impact

The retrieveDeposit function is supposed to enable a user to trigger the fallback function in the branchbidgeagent. this fallback function then sets the status of a deposit to failed, whereby user can then redeem his deposit. however due to the omnichain environment the fallback function might fail to trigger in some edge cases. thats why the developers intended to use the retrievedeposit function to force the fallback function to fire which would then set status of deposit as failed, so user can retrieve his deposit.

The problem is any call from the bridge to the root has a certain sequence of events that is missing in this case. the sequence includes depositing the msg.value to the wrappedNativeToken to use as gas, this is usually done via this line:

//Wrap the gas allocated for omnichain execution.
wrappedNativeToken.deposit{value: msg.value}();

this should be done in the retrieveDeposit function, however it is missing.

the retrieveDeposit function then calls _sendRetrieveOrRetry, which in turn calls _createGasDeposit which deposits Gas for call. this in turn calls _depositGas which then transfers the wrapped native token the user supplied from the branch bridge agent contract to the branch port contract:

 address(wrappedNativeToken).safeTransfer(localPortAddress, _gasToBridgeOut);

However this attempt to transfer from the branch bridge agent contract to the branch port will always fail because the retrieveDeposit function is missing the step where the gas is wrapped to the native token(ie is deposited into the balance of the branch bridge agent).

The consequence of this is that if the fallback function does not trigger during the the initial deposit request, a user will not be able to force the fallback to trigger via the retrieveDeposit function, which basically means he can not redeem his deposit because its status will still be succesful.

Proof of Concept

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

    //Prepare data
    bytes memory packedData;

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

        //Mock action
        calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

        //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, ftmChainId);

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

    address _user = address(this);

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

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

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

    //Prepare deposit info
    DepositParams memory depositParams = DepositParams({
        hToken: address(avaxMockAssethToken),
        token: address(avaxMockAssetToken),
        amount: 150 ether,
        deposit: 100 ether,
        toChain: ftmChainId,
        depositNonce: 1,
        depositedGas: 1 ether
    });

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

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

    console2.log("BALANCE BEFORE:");
    console2.log("User avaxMockAssetToken Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
    console2.log("User avaxMockAssethToken Balance:", MockERC20(avaxMockAssethToken).balanceOf(_user));

    //Set MockAnycall AnyFallback mode ON
    MockAnycall(localAnyCallAddress).toggleFallback(1);

    require(avaxMockAssetToken.balanceOf(address(avaxPort)) == 0, "balance of port is not zero");

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

    // @audit Retrieve Deposit, will fail with  TransferFailed() error.
    avaxMulticallBridgeAgent.retrieveDeposit{value: 1 ether}(depositParams.depositNonce);

}

Tools Used

Manual Review

Recommended Mitigation Steps]

if you update the retrieve deposit function as below, the test above should pass.

    function retrieveDeposit(uint32 _depositNonce) external payable lock requiresFallbackGas {
    //Encode Data for cross-chain call.
    bytes memory packedData = abi.encodePacked(bytes1(0x08), _depositNonce, msg.value.toUint128(), uint128(0));
    
    // @audit Wrap the gas allocated for omnichain execution.
     wrappedNativeToken.deposit{value: msg.value}();

    //Update State and Perform Call
    _sendRetrieveOrRetry(packedData);
}

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/BranchBridgeAgent.sol#L531-L543

Vulnerability details

Impact

the retrysettlement function allows a user to retry a settlement that failed. there are two methods to trigger this action, either from the root bridge or from the root bridge. The functionality works fine on the root side but fails when triggered from the branch, which is is more convenient for a user. this is because the function did not wrap the gas the user provided that is allocated for omnichain execution(msg.value is deposited on behalf of root bridge account). so when the the branch bridge agent attempts to transfer the wrapped native token to the port, the transaction fails.

function _depositGas(uint128 _gasToBridgeOut) internal virtual {
    address(wrappedNativeToken).safeTransfer(localPortAddress, _gasToBridgeOut);
}

Proof of Concept

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

    //Prepare data
    bytes memory packedData;

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

        //Mock action
        calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

        //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, ftmChainId);

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

    address _user = address(this);

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

    //assure there is enough balance for mock action
    hevm.prank(address(rootPort));
    ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(address(rootPort), 50 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,
        toChain: ftmChainId
    });

    console2.log("BALANCE BEFORE:");
    console2.log("User avaxMockAssetToken Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
    console2.log("User avaxMockAssethToken Balance:", MockERC20(avaxMockAssethToken).balanceOf(_user));

    //Set MockAnycall AnyFallback mode ON
    MockAnycall(localAnyCallAddress).toggleFallback(1);

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

    //Set MockAnycall AnyFallback mode OFF
    MockAnycall(localAnyCallAddress).toggleFallback(0);

    //Perform anyFallback transaction back to root bridge agent
    MockAnycall(localAnyCallAddress).testFallback();

    uint256 _amount = 150 ether;
    uint256 _deposit = 100 ether;
    uint256 _amountOut = 150 ether;
    uint256 _depositOut = 150 ether;
    console2.log("DATA");
    console2.log(_amount);
    console2.log(_deposit);
    console2.log(_amountOut);
    console2.log(_depositOut);

    uint32 settlementNonce = multicallBridgeAgent.settlementNonce() - 1;

    Settlement memory settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);

    console2.log("Status after fallback:", settlement.status == SettlementStatus.Failed ? "Failed" : "Success");

    require(settlement.status == SettlementStatus.Failed, "Settlement status should be failed.");

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

    // @audit Retry Settlement from branch chain
    avaxMulticallBridgeAgent.retrySettlement{value: 1 ether}(settlementNonce, 0.5 ether);

    settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);

    require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");
}

Tools Used

Manual Review

Recommended Mitigation Steps

update the retrysettlement function as below, the POC above will pass:

function retrySettlement(uint32 _settlementNonce, uint128 _gasToBoostSettlement)
    external
    payable
    lock
    requiresFallbackGas
{
    //Encode Data for cross-chain call.
    bytes memory packedData = abi.encodePacked(
        bytes1(0x07), depositNonce, _settlementNonce, msg.value.toUint128(), _gasToBoostSettlement
    );

    // @audit Wrap the gas allocated for omnichain execution.
     wrappedNativeToken.deposit{value: msg.value}();

    //Update State and Perform Call
    _sendRetrieveOrRetry(packedData);
}

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/RootBridgeAgent.sol#L1141-L1156

Vulnerability details

Impact

The purpose of the retrieveDeposit function is to enable a user to be able to redeem a deposit he entered into the system. the mechanism works based on the promise that this function will be able to forcefully make the root bridge agent trigger the fallback function.

            if (!executionHistory[fromChainId][uint32(bytes4(data[1:5]))]) {
            //Toggle Nonce as executed
            executionHistory[fromChainId][nonce] = true;

            //Retry failed fallback
            (success, result) = (false, "")

by returning false, the anycall contract will attempt to trigger the fallback function in the branch bridge, which would in turn set the status of the deposit as failed. the user can then redeem his deposit because its status is now failed.

    function redeemDeposit(uint32 _depositNonce) external lock {
    //Update Deposit
    if (getDeposit[_depositNonce].status != DepositStatus.Failed) {
        revert DepositRedeemUnavailable();
    }

The problem is that according to how anycall protocol works, it is completely feasible that the execution in root bridge completes succesfully, but the fallback in branch might still fail to execute.

 uint256 internal constant MIN_FALLBACK_RESERVE = 185_000; // 100_000 for anycall + 85_000 fallback execution overhead

for example, the anycall to the rootbridge might succeed due to enough gas stipend, while the fallback execution fails due to low gas stipend.

if this is the case then the deposit nonce would be stored in the executionHistory during the initial call, so when the retrievedeposit call is made it, it would think that the transaction is already completed, which would trigger this block instead:

               _forceRevert();
            //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
            return (true, "already executed tx");

The impact of this is that if the deposit transaction is recorded in root side as completed, a user will never be able to use retrievedeposit function redeem his deposit from the system.

Proof of Concept

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

    //Prepare data
    bytes memory packedData;

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

        //Mock action
        calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData:   ""});

        //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, ftmChainId);

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

    address _user = address(this);

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

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

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

    //Prepare deposit info
            //Prepare deposit info
    DepositParams memory depositParams = DepositParams({
        hToken: address(avaxMockAssethToken),
        token: address(avaxMockAssetToken),
        amount: 150 ether,
        deposit: 100 ether,
        toChain: ftmChainId,
        depositNonce: 1,
        depositedGas: 1 ether
    });

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

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

    console2.log("BALANCE BEFORE:");
    console2.log("User avaxMockAssetToken Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
    console2.log("User avaxMockAssethToken Balance:",  MockERC20(avaxMockAssethToken).balanceOf(_user));

    require(avaxMockAssetToken.balanceOf(address(avaxPort)) == 0, "balance of port is not zero");

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

    avaxMulticallBridgeAgent.retrieveDeposit{value: 1 ether}(depositParams.depositNonce);


    // fallback is not triggered.

    // @audit Redeem Deposit, will fail with   DepositRedeemUnavailable()
    avaxMulticallBridgeAgent.redeemDeposit(depositParams.depositNonce);

}

Tools Used

Manual Review

Recommended Mitigation Steps

just make the root bridge return (false, "") regardles of whether the transaction linked to the original deposit was completed or not.

 /// DEPOSIT FLAG: 8 (retrieveDeposit)
else if (flag == 0x08) {
        (success, result) = (false, "");

to avoid also spamming the usage of the retrievedeposit function, it is advisable to add a check in the retrieveDeposit function to see whether the deposit still exists, it doesnt make sense to try and retrieve a deposit that has already been redeemed.

    function retrieveDeposit(uint32 _depositNonce) external payable lock requiresFallbackGas {
    address depositOwner = getDeposit[_depositNonce].owner;
    
    if (depositOwner == address(0)) {
        revert RetrieveDepositUnavailable();
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/BranchBridgeAgent.sol#L1061-L1085

Vulnerability details

Impact

the gas management systemin ulysses relies in a complex set of actions undertaken in source destination, root and destination chain. for a succesful transaction, the gas cost is usually deposited in the branch port as wrapped native token. this gas cost is divided into cost of gas to process transaction in root, and cost of gas to process transaction in destination chain. the user specifies how much of this gas will be used in destination chain via the gastobridgeout variable. that should always be lower than the actual gas they deposited. any difference between these two amounts is considered protocol fees that are accumulated in root via the _payExecutionGas function. the destination chain would receive the request from the root along with how much gas is needed to process interaction, would then pull native tokens from its local port to process transaction. if the amount used to process transaction in destination is more than what the user specified, the excess is paid back to the user. to sum up here is the simple summary of the gas flow in system:

1. branch port native token supply increases by x amount of gas.
2. root port mints global representation of native token deposited in source.
3. root port burns global representation of gas to be used in destination chain.
4. destination port utilizes this gas and transfers out excess to user.

this ensures that any gas deposited in source chain by user is reflected in root and also the reverse and equal operation occurs for the gas to bridge out to destination.

The problem in my opinion occurs if the transaction between source and root fails for some reason, this would trigger the anyfallback in the branch bridge. the branch bridge would then withdraw fallback gas costs(minimum execution cost) from the local port, replenish the local anycall proxy with this amount, and effectively keep the remainder of gas the user actually deposited to be utilized for the succesfull transaction in destination.

The local port essentially keeps any user provided excess gas in the system. Now , the question is this part of system design, ie this is considered some sort of protocol fee earned by branch port or assets locked in system to be used by protocol? and if so, why is this not communicated clearly in docs or code? the docs state "When interacting with a Port to perform any of these actions, no protocol fees will be charged from user's assets". there is some confusion here.

this is deemed a critical issue because if this excess gas cost from failed transactions increases the supply of native tokens in branch ports, this can incentivize malicious actors who have deployed branch bridge agents to find a way to force more transactions to fail to increase the supply of native tokens in ports, this is potentially possible via the _gasSwapOut function in the root bridge agent which does not revert the entire transaction but will instead trigger the fallback in source bridge, i will discuss this related issue in a detailed and seperate issue i will submit. i understand that at the moment there is no way to withdraw these funds by those malicious actors, but this will be possible once port strategies are introduced. port strategies will effectively enable PortStrategy contracts to have the permission to manage a portion of the locked assets in local ports to earn yield, although its unclear now since they are not yet implemented.

Another issue is that this gas cost which is essentially sent as a value with the transaction (msg.value), could be arbitrarily large, this could be due to an error by the user. for example ,lets say the user erroneously passed 100 ether to be used as gas. if the transaction is succesful and if he keeps the difference between actual gas and gas to bridgeout minimal, he would retrieve all the reminder gas in the destination chain.

However if the transaction fails, all the gas he deposited is essentially locked in source branch port. The POC below highlights this.

Proof of Concept

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

    //Prepare data
    bytes memory packedData;

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

        //Mock action
        calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

        //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, ftmChainId);

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

    address _user = address(this);

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

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

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

    //Prepare deposit info
            //Prepare deposit info
    DepositParams memory depositParams = DepositParams({
        hToken: address(avaxMockAssethToken),
        token: address(avaxMockAssetToken),
        amount: 150 ether,
        deposit: 100 ether,
        toChain: ftmChainId,
        depositNonce: 1,
        depositedGas: 1 ether
    });

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

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

    console2.log("BALANCE BEFORE:");
    console2.log("User avaxMockAssetToken Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
    console2.log("User avaxMockAssethToken Balance:", MockERC20(avaxMockAssethToken).balanceOf(_user));

    //Set MockAnycall AnyFallback mode ON
    MockAnycall(localAnyCallAddress).toggleFallback(1);

    require(avaxMockAssetToken.balanceOf(address(avaxPort)) == 0, "balance of port is not zero");

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

    // Call 'anyFallback'
    hevm.prank(localAnyCallExecutorAddress);
    avaxMulticallBridgeAgent.anyFallback(anyFallbackData);
    // @audit the excess gas remains in the port, this will keep adding up everytime
    // a transaction fails and the user transfered gas more than the execution cost
    // there is no proper accounting or a mechanism to sweep or redeem this accumulated amount from the local port.
    require(avaxMockAssetToken.balanceOf(address(avaxPort)) > 40 ether, "balance of port is less than excess gas sent");
}

Tools Used

Manual Review

Recommended Mitigation Steps

There needs to be a mechanism in the branch bridge payfallback process similar to that done in the payexecution process for the gas bridged out, whereby excess gas is returned to the user in case anyfallback is triggered. Care must also be taken to burn any representation of this gas that was minted in root, before triggering the fallback in branch.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/uni-v3-staker/UniswapV3Staker.sol#L373-L374

Vulnerability details

Impact

the uniswap v3 staker has a restake functionality. according to natspec in the code, anyone would be able to restake if the block time is after the end time of the incentive. this condition is enforced via the code block below which is part of unstaking.

    // anyone can call restakeToken if the block time is after the end time of the incentive
    if ((isNotRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();

However this is not the case, the condition will always revert. this is because isNotRestake is set to true within the restake function as seen below:

    function restakeToken(uint256 tokenId) external {
    IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
    if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true); // @audit is set to true

    (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
        NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

    _stakeToken(tokenId, pool, tickLower, tickUpper, liquidity);
}

since its true, the output of condition will resolve to : (True || False) && True = True

i believe the initial intention of developers was to pass it as false. however with regards to impact and implications, as of this writing it is unclear what the original intention of this functionality was. but in all cases it clearly does do what its supposed to according natspecs in the code.

Proof of Concept

    function testFullIncentiveRestake() public {
    // Create a Uniswap V3 pool
    (pool, poolContract) =
        UniswapV3Assistant.createPool(uniswapV3Factory, address(token0), address(token1), poolFee);

    // Initialize 1:1 0.3% fee pool
    UniswapV3Assistant.initializeBalanced(poolContract);
    hevm.warp(block.timestamp + 100);

    // 3338502497096994491500 to give 1 ether per token with 0.3% fee and -60,60 ticks
    uint256 tokenId = newNFT(-60, 60, 3338502497096994491500);

    uint256 minWidth = 10;
    // Create a gauge
    gauge = createGaugeAndAddToGaugeBoost(pool, minWidth);

    // Create a Uniswap V3 Staker incentive
    key = IUniswapV3Staker.IncentiveKey({pool: pool, startTime: IncentiveTime.computeEnd(block.timestamp)});

    uint256 rewardAmount = 1 ether;
    rewardToken.mint(address(this), rewardAmount);
    rewardToken.approve(address(uniswapV3Staker), rewardAmount);

    createIncentive(key, rewardAmount);

    rewardToken.mint(address(this), 1 ether);
    rewardToken.approve(address(bHermesToken), 1 ether);
    bHermesToken.deposit(1 ether, address(this));
    bHermesToken.claimBoost(1 ether);
    hevm.warp(key.startTime);

    // Transfer and stake the position in Uniswap V3 Staker
    nonfungiblePositionManager.safeTransferFrom(address(this), address(uniswapV3Staker), tokenId);

    // Check that the position is in Uniswap V3 Staker
    assertEq(nonfungiblePositionManager.ownerOf(tokenId), address(uniswapV3Staker));
    (address owner,,, uint256 stakedTimestamp) = uniswapV3Staker.deposits(tokenId);
    assertEq(owner, address(this));
    assertEq(stakedTimestamp, block.timestamp);

    hevm.warp(block.timestamp +  9 days);

    (uint256 reward,) = uniswapV3Staker.getRewardInfo(key, tokenId);

    hevm.prank(address(1));
    uniswapV3Staker.restakeToken(tokenId);
}

the poc above simulates a request to restake after incentive period ends, it reverts with NotCalledByOwner() which is not what developers initially intended.

Tools Used

Manual Review

Recommended Mitigation Steps

        function restakeToken(uint256 tokenId) external {
    IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
    if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false); // @audit

    (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
        NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

    _stakeToken(tokenId, pool, tickLower, tickUpper, liquidity);
}

updating to fix above will enable anyone to restake after incentive period ends and it will not revert with NotCalledByOwner().

in addition, you will need verify whether this functionality to restake position by anyone is required in the first place and whether it first with overall business logic. it seems this feature was added but not yet tested properly.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/MulticallRootRouter.sol#L175-L236

Vulnerability details

Impact

ulyssses omnichain provides the user with the ability to do what is known as multicall transactions. this is possible via the multicallrouter which from my review of code base, is the protocols primarily method for enabling omnichain transactions between source and destination chains. the contract enables the user who is in the source chain, lets say avax, to do multicalls in the root chain via their virtual account, withdraw funds from their virtual account and then use these funds to do multiple settlements(or a single settlement) in destination chain which could be FTM for instance,and lets them retrieve their desired output tokens based on amounts they deposited, all of this within a single transaction.

There are multiple endpoints exposed to enable these multicall functionalities, one of them enables what is known as a multicallmultioutputnodeposit or multicallsingleoutputnodeposit. they are exposed to branch bridge agents via calling 0x01 flag to signal a transaction without a deposit. this in turn reaches the root, and triggers the executeNoDeposit function in the rootbranchbridgeexecutor contract. that function then fires the anyExecute function in the multicallrouter contract.

    function anyExecute(bytes1 funcId, bytes calldata encodedData, uint24)
    external
    payable
    override
    lock
    requiresExecutor
    returns (bool, bytes memory)
{

based on the payload set by user from the source chain, the function will do a number of things, first it determines the type of transaction via the flag, lets assume the user chose the multicallMultipleOutput, which signals he wants to do multiple calls within root environment, and then finally he wants to do multiple settlements in destination chain. this would trigger the code block below:

            /// FUNC ID: 2 (multicallSingleOutput)
    } else if (funcId == 0x02) {
        (IMulticall.Call[] memory callData, OutputParams memory outputParams, uint24 toChain) =
            abi.decode(encodedData, (IMulticall.Call[], OutputParams, uint24));

        _multicall(callData);

        _approveAndCallOut(
            address(0), // @audit should this be address of user who initiates request?, so that he can retry a settlement that failed to fire fallback
            outputParams.recipient,
            outputParams.outputToken,
            outputParams.amountOut,
            outputParams.depositOut,
            toChain
        );

        /// FUNC ID: 3 (multicallMultipleOutput)
    } else if (funcId == 0x03) {
        (IMulticall.Call[] memory callData, OutputMultipleParams memory outputParams, uint24 toChain) =
            abi.decode(encodedData, (IMulticall.Call[], OutputMultipleParams, uint24));

        _multicall(callData);

        _approveMultipleAndCallOut(
            address(0), // @audit should this be address of user who initiates request?, so that he can retry a settlement that failed to fire fallback
            outputParams.recipient,
            outputParams.outputTokens,
            outputParams.amountsOut,
            outputParams.depositsOut,
            toChain
        );
        /// UNRECOGNIZED FUNC ID
    }

the problem manifests in the code block above, essentially because the owner of the settlement that needs to be cleared in destination branch will be the zero address. depending on whether user requested a multicallSingleOutput or a multicallMultiOutput action, this code block will do a number of things, first it will allow the user to make multi calls via payload he specified, second it will approve the Root Port to spend output hTokens on behalf of user. it will then move output hTokens from Root to destination Branch and call 'clearTokens'. this process updates the state of the root bridge via the _updateStateOnBridgeOut. the tokens are then 'cleared' in destination chain, and user should receive his desired output tokens.

However, because the settlement has no linked owner, if the transaction to destination chain fails for whatever reason, the user will be unable to retry the settlement via the root bridge, and they also cannot redeem the settlement. this effectively means the user funds transfered to ulysses root environment are essentialy locked.

    function redeemSettlement(uint32 _depositNonce) external lock {
    //Get deposit owner.
    address depositOwner = getSettlement[_depositNonce].owner;

    //Update Deposit
    if (getSettlement[_depositNonce].status != SettlementStatus.Failed || depositOwner == address(0)) {
        revert SettlementRedeemUnavailable();

as you can see above, a settlement with owner set to address zero is not redeemable. the logic behind that was a redeemed settlement will be deleted and hence getSettlement would retrieve an empty settlement struct with an owner of address zero.

function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
    //Get Settlement
    Settlement memory settlement = getSettlement[_settlementNonce];

    //Check if Settlement hasn't been redeemed.
    if (settlement.owner == address(0)) return false;

as you can see above, settlement retries will also fail, because once again, the settlement was set with owner of address zero initially.

the impact of this is very high in my opinion because not only will user funds be permanently locked, but system invariants will be broken since the token accounting in system will not be in balance. proof of concept will help clarify this issue further.

Proof of Concept

    function testMulticallMultipleOutputNoDepositFailed() public {
    //Add Local Token from Avax
    testSetLocalToken();

    require(
        RootPort(rootPort).getLocalTokenFromGlobal(newAvaxAssetGlobalAddress, ftmChainId) == newAvaxAssetLocalToken,
        "Token should be added"
    );

    
    hevm.deal(address(userVirtualAccount), 1 ether);
    hevm.deal(address(avaxMulticallBridgeAgentAddress), 10 ether);

    //Prepare data
    address[] memory outputTokens = new address[](2);
    uint256[] memory amountsOut = new uint256[](2);
    uint256[] memory depositsOut = new uint256[](2);
    bytes memory packedData;

    {
        outputTokens[0] = ftmGlobalToken;
        outputTokens[1] = newAvaxAssetGlobalAddress;
        amountsOut[0] = 100 ether;
        amountsOut[1] = 100 ether;
        depositsOut[0] = 50 ether;
        depositsOut[1] = 0 ether;

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

        //Prepare call to transfer 100 wFTM global token from contract to Root Multicall Router
        calls[0] = Multicall2.Call({
            target: ftmGlobalToken,
            callData: abi.encodeWithSelector(bytes4(0x23b872dd), address(userVirtualAccount), address(rootMulticallRouter), 100 ether)
        });

        //Prepare call to transfer 100 hAVAX global token from contract to Root Multicall Router
        calls[1] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0x23b872dd), address(userVirtualAccount), address(rootMulticallRouter), 100 ether)
        });


        //Output Params
        OutputMultipleParams memory outputMultipleParams =
            OutputMultipleParams(userVirtualAccount, outputTokens, amountsOut, depositsOut);

        // minted assets to the user directly
        hevm.startPrank(address(rootPort));
        ERC20hTokenRoot(ftmGlobalToken).mint(userVirtualAccount, 100 ether, ftmChainId);
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(userVirtualAccount, 100 ether, avaxChainId);
        hevm.stopPrank();


        uint256 balanceUserBeforeAvax = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
        uint256 balanceUserBeforeFtm =  MockERC20(ftmGlobalToken).balanceOf(userVirtualAccount);
        require(balanceUserBeforeAvax == 100 ether, "User Balance should be 100 avax");
        require(balanceUserBeforeFtm == 100 ether, "User Balance should be 100 ftm");

        //User Approves spend by multicall contract
        hevm.startPrank(address(userVirtualAccount));
        MockERC20(ftmGlobalToken).approve(address(rootMulticallRouter.multicallAddress()), 100 ether);
        MockERC20(newAvaxAssetGlobalAddress).approve(address(rootMulticallRouter.multicallAddress()), 100 ether);
        hevm.stopPrank();

        //toChain
        uint24 toChain = ftmChainId;

        //RLP Encode Calldata
        bytes memory data = abi.encode(calls, outputMultipleParams, toChain);

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



    uint256 balanceBeforePortAvax = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
    uint256 balanceBeforePortFtm = MockERC20(ftmGlobalToken).balanceOf(address(rootPort));


    
    //Call Deposit function
    encodeCallNoDeposit(
        payable(avaxMulticallBridgeAgentAddress),
        payable(multicallBridgeAgent),
        1,
        packedData,
        0.0001 ether,
        0.00005 ether,
        avaxChainId
    );


    uint256 balanceUserAfterAvax = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
    uint256 balanceUserAfterFtm = MockERC20(ftmGlobalToken).balanceOf(userVirtualAccount);
    require(balanceUserAfterAvax == 0 ether, "User Balance should be 0 global avax");
    require(balanceUserAfterFtm == 0 ether, "User Balance should be 0 global ftm");


    uint256 balanceAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootMulticallRouter));
    uint256 balanceFtmAfter = MockERC20(ftmGlobalToken).balanceOf(address(rootMulticallRouter));

    require(balanceAfter == 0, "Router Balance should be cleared");
    require(balanceFtmAfter == 0, "Router Balance should be cleared");


    uint256 balanceAfterPortAvax = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
    uint256 balanceAfterPortFtm = MockERC20(ftmGlobalToken).balanceOf(address(rootPort));

    require(balanceAfterPortAvax ==  balanceBeforePortAvax + 100 ether, "Root port global avax Balance should be increased by 100");
    require(balanceAfterPortFtm ==  balanceBeforePortFtm + 50 ether, "Root port global ftm Balance should be increased by 50");

    uint32 settlementNonce = 1;

    Settlement memory settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);

    console2.log("Status after fallback:", settlement.status == SettlementStatus.Failed ? "Failed" : "Success");

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

   
    bytes memory anyFallbackData = abi.encodePacked(bytes1(0x01), address(userVirtualAccount), uint32(settlementNonce), packedData, uint128(0.0001 ether), uint128(0.00005 ether));
    hevm.prank(localAnyCallExecutorAddress);
    multicallBridgeAgent.anyFallback(anyFallbackData);

    Settlement memory settlement3 = multicallBridgeAgent.getSettlementEntry(settlementNonce);

    console2.log("Status after fallback:", settlement3.status == SettlementStatus.Failed ? "Failed" : "Success");

    require(settlement3.status == SettlementStatus.Failed, "Settlement status should be failed after fallback.");

    //Attempt to Redeem settlement since its now in failed state via fallback
    hevm.startPrank(address(userVirtualAccount));
    // @audit this will fail with SettlementRedeemUnavailable() since settlement has no owner
    multicallBridgeAgent.redeemSettlement(settlementNonce);
    hevm.stopPrank();


}

The POC above demonstrates in detail how this problem develops. in this single transaction it is possible for a user to leverage the multicall feature to transfer their own funds to the rootMulticallRouter, which would then proceed to attempt to settle transactions in the destination chain the user chose.

The following discussion is based on POC inputs:

For FTM , user is requesting an amount of 100 and deposit of 50, this will cause the root bridge agent to update its state, which will effectively increase ts port balance of FTM global by 50, it will also effectively burn the remaining 50 that is in the multi router.

For Avax Global , user is requesting an amount of 100 and deposit of 0, this will cause the root bridge agent to update its state, which will effectively increase its port balance of AVAX global by 100, the full amount.

If the transfer to FTM bridge agent is successful, it will settle the transaction for the user in the destination chain, which will do the following:

For FTM global, it will bridge in(mint) 50 local FTM tokens for the user. it will also withdraw 50 global tokens and give them to the user. so the user ends up with the same amount of tokens he started with, except he now has 50 global FTM and 50 local FTM.

For AVAX global, it will bridge in or mint 100 local AVAX tokens for the user. so the user ends up with the same amount of tokens he started with, except he now has 100 local AVAX tokens.

But what if the calloutandbridge from root to branch bridge agent fails, maybe to low gas. The anyfallback if fired successfully in root bridge, will enable a user to either retry or redeem the settlement. The problem is the _approveAndCallOut function the multicallrootrouter set the owner to the zero address. which means the owner of the settlement will be the zero address. This means the user will have no way to retry or redeem his settlement in the destination branch branch. This effectively means the user has lost the entire amount of global AVAX and FTM he initially deposited with the router to process his request. Not only that but the token accounting in the system will not be in balance.for example the total amount of global AVAX in the system will not equal the total amount of local AVAX, hence the invariant of 1:1 supply is broken. The broken invariant applies to FTM as well. specifically ftm global > ftm local by 50, and AVAX global > AVAX local by 100.

Tools Used

Manual Review

Recommended Mitigation Steps

            _approveAndCallOut(
            outputParams.recipient, // @audit: fixed here by updating the owner of settlement
            outputParams.recipient,
            outputParams.outputToken,
            outputParams.amountOut,
            outputParams.depositOut,
            toChain
        );

run the poc again with this modification, it should pass.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/78e49c651fd119b85bb79296620d5cb39efa7cdd/src/ulysses-omnichain/RootPort.sol#L406-L410

Vulnerability details

Impact

the addbridgeagentfactory function is responsible for adding a new bridge agent factory to the root port.

However the current implementation is faulty. the faulty logic is in the following line:

bridgeAgentFactories[bridgeAgentsLenght++] = _bridgeAgentFactory;

a couple of problems here, the function is attempting to access an index that does not yet exist in the bridgeAgentFactories array, this should return an out of bounds error. the function also does not update the isBridgeAgentFactory mapping. once a a new bridge agent factory is added, a new dict item with key equal to address of new bridge agent factory and value of true. this mapping is then used to enable toggling the factory, ie enabling or disabling it via the toggleBridgeAgentFactory function.

Impact: the code hints that this is a key governance action. it does not work at the moment however with regards to impact, at this moment it is unclear from the code what the overall impact would be to the functioning of the protocol, that is why it is rated as medium rather than high. feedback from sponsors is welcome to determine severity.

Proof of Concept

    function testAddRootBridgeAgentFactoryBricked() public {
    //Get some gas
    hevm.deal(address(this), 1 ether);

    RootBridgeAgentFactory newBridgeAgentFactory = new RootBridgeAgentFactory(
        ftmChainId,
        WETH9(ftmWrappedNativeToken),
        localAnyCallAddress,
        address(ftmPort),
        dao
    );

    rootPort.addBridgeAgentFactory(address(newBridgeAgentFactory));
    
    require(rootPort.bridgeAgentFactories(0)==address(bridgeAgentFactory), "Initial Factory not in factory list");
    require(rootPort.bridgeAgentFactories(1)==address(newBridgeAgentFactory), "New Factory not in factory list");

}

the above POC demonstrates this, it attempts to call the function in question, and returns an "Index out of bounds" error.

Tools Used

Recommended Mitigation Steps

    function addBridgeAgentFactory(address _bridgeAgentFactory) external onlyOwner {
    // @audit this function is broken
    // should by implemented as so
    isBridgeAgentFactory[_bridgeAgentFactory] = true;
    bridgeAgentFactories.push(_bridgeAgentFactory);
    bridgeAgentFactoriesLenght++;

    emit BridgeAgentFactoryAdded(_bridgeAgentFactory);
}

the correct implementation is above, this is also identical to how the branch ports implement this functionality.

Assessed type

Governance