Maia DAO - Ulysses - Udsen's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreRootRouter.sol#L481-L487 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L266-L267

Vulnerability details

Impact

The CoreRootRouter._setLocalToken function is used to set the local token on a specific chain for a global token. The function initially checks whether the local token is is already added as shown below:

    if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();

If the token is not already added then the global token's new branch chain address is set as shown below:

    IPort(rootPortAddress).setLocalAddress(_globalAddress, _localAddress, _dstChainId);

The IPort.setLocalAddress function performs two mapping state updates as shown below:

    getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
    getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;

Now let's consider the following scenario:

1. Assume _globalAddress = 0x01, _localAddress = 0x02, _srcChainId = 1.

2. The CoreRootRouter._setLocalToken function is called with above input parameter values.

3. The getGlobalTokenFromLocal[0x02][1] is equal to address(0) since it is not initialized.

4. Hence the IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId) condition will return false and the _setLocalToken function will proceed executing.

5. But the getLocalTokenFromGlobal[0x01][1] has value 0x03 since it is already assigned during a previous call to the CoreRootRouter._setLocalToken function.

6. There is no check to verify whether getLocalTokenFromGlobal[_globalAddress][_srcChainId] is already set.

7. Hence the transaction execution will proceed to the IPort.setLocalAddress function and will update the getGlobalTokenFromLocal and getLocalTokenFromGlobal for the corresponding values provided.

8. This will overwrite the existing value of ``getLocalTokenFromGlobal[0x01][1]which is0x03with the new passed in_localAddressvalue0x02` as shown below:

    getLocalTokenFromGlobal[0x01][1] = 0x02;
   

But this is not the expected behaviour of the IPort.setLocalAddress function since above vulnerability could overwrite the getLocalTokenFromGlobal mapping values for already set _globalAddress and _srcChainId pairs. This can break the internal logic execution and accounting of the hTokens defined in the system.

Proof of Concept

    function _setLocalToken(address _globalAddress, address _localAddress, uint16 _dstChainId) internal {
        // Verify if the token already added
        if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();

        // Set the global token's new branch chain address
        IPort(rootPortAddress).setLocalAddress(_globalAddress, _localAddress, _dstChainId);
    }

https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreRootRouter.sol#L481-L487

        getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
        getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L266-L267

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to perform an additional check in the CoreRootRouter._setLocalToken to ensure that the getLocalTokenFromGlobal mapping value for the _globalAddress and _srcChainId pair is not already set as shown below:

    if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();
    if (IPort(rootPortAddress).isGlobalToken(_globalAddress, _dstChainId)) revert TokenAlreadyAdded();     

Assessed type

Other

1. _refundee VARIABLE HAS REDUNDANT DECLARATION AS payable

The BranchBridgeAgent._performCall function is used to perform calls to LayerZero messaging layer Endpoint for cross-chain messaging. The _refundee is a payable address which is used to refund excess gas to. The _refundee is declared as a payable address when passed in as an input parameter to the _performCall function. The issue is _refundee is again recasted to a payable address inside the call to the layerzero endpoint which is not required as shown below:

    ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}( //@audit-info - uses layer zero messaging layer endpoint for cross-chain messaging
        rootChainId,
        rootBridgeAgentPath,
        _payload,
        payable(_refundee),
        address(0),
        abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
    );

Hence it is recommneded to remove the redundant recasting of the _refundee address to a payable address inside the call to the layerzero endpoint for cross chain messaging.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L774 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L919

2. LACK OF INPUT VALIDATION CHECK ON address(0) COULD LEAD TO LOSS OF FUNDS

The BranchBridgeAgent._clearToken is an internal function which is used to request balance clearance from a Port to a given user. The _clearToken is called by the BranchBridgeAgent.clearToken function by passing in the _recipient address to whom the balance clearance should be sent to.

The issue is during the execution flow of the BranchBridgeAgent.clearToken function there is no check to verifty whether the _recipient != address(0). Hence if the address(0) is passed in as the _recipient address the balance clearance will be sent to the address(0) which is loss of funds.

The _clearToken function calls the IPort.bridgeIn function and IPort.withdraw function. Inside the bridgeIn function and withdraw function ERC20 _mint and safeTransfer functions are used. But event inside these functions no address(0) check is performed on the passed in _recipient address.

Hence it is recommended to perform the input validation check inside the BranchBridgeAgent._clearToken function to ensure the passed in _recipient address is not equal to address(0). It is recommended to add the following line of code at the beginning of the _clearToken function.

require( _recipient != address(0), "_recipient can not be address(0)");

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L906-L914 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L494-L571

3. _recipient ADDRESS IS NOT DECLARED AS A payable ADDRESS IN THE BranchBridgeAgentExecutor.executeWithSettlement FUNCTION

The BranchBridgeAgentExecutor.executeWithSettlement function is used to execute a cross-chain request with a single settlement. The function accepts the _recipient address which is the the recipient of the settlement as shown below:

function executeWithSettlement(address _recipient, address _router, bytes calldata _payload)

But the issue is _recipient is not declared as a payable address. Since _recipient is expected to receive the native ETH it is recommneded to declare the _recipient address as a payable address in the executeWithSettlement function signature. The modified line of code is shown below:

function executeWithSettlement(address payable _recipient, address _router, bytes calldata _payload)

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgentExecutor.sol#L66

4. BranchPort._bridgeOut TRANSACTION COULD REVERT IF THE _amount < _deposit

The BranchPort._bridgeOut is an internal function used to bridge out hTokens and underlying tokens. Within the function implementation the _hTokenAmount to bridge out is calculated as follows:

    uint256 _hTokenAmount = _amount - _deposit; 

The issue here is that if the _amount < _deposit the arithmetic operation will revert due to underflow error but the cause for the revert will not be clear. Hence it is recommended to add the below conditional check to verify (_amount > _deposit) before calculating the _hTokenAmount.

    require(_amount > _deposit, "Arithmetic Underflow");
    uint256 _hTokenAmount = _amount - _deposit; 

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L522

5. THE settlement.status IS BY DEFAULT SET TO THE STATUS_SUCCESS STATE SINCE THE STATUS_SUCCESS == 0

The RootBridgeAgent.redeemSettlement function is used to enable an user to redeem thier settlement. Here initially the settlement.status is checked to ensure whether the settlement is redeemable as shown below:

    if (settlement.status == STATUS_SUCCESS) revert SettlementRedeemUnavailable();

The STATUS_SUCCESS is declared in the BridgeAgentConstant.sol as shown below:

uint8 internal constant STATUS_SUCCESS = 0;

At the end of the RootBridgeAgent.redeemSettlement function the settlement corresponding to the _settlementNonce is deleted as shown below:

    delete getSettlement[_settlementNonce]; 

Due to the above deletion the settlement.status will be equal to 0 which is the default value. Hence as a result this is still equal to the STATUS_SUCCESS state. Hence even for any non-existing settlement and for a settlement that has already being deleted the settlement.status will be equal to the STATUS_SUCCESS state.

Hence it is recommended to update the constant STATUS_SUCCESS = 2 because in that case the default value of the settlement.status will not be equal to the STATUS_SUCCESS state by default.

https://github.com/code-423n4/2023-09-maia/blob/main/src/interfaces/BridgeAgentConstants.sol#L23 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L307 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L343

6. LACK OF INPUT VALIDATION CHECK OF address(0) FOR THE _recipient ADDRESS IN THE RootBridgeAgent.bridgeIn FUNCTION

The RootBridgeAgent.bridgeIn function is used to bridge in hTokens from branch to root. The bridgeIn function calls the IPort.bridgeToRoot function by passing in the _recipient address to whom the _amount - _deposit amount of _hTokens will be transferred to and the _deposit amount of _hTokens will be minted to inside the bridgeToRoot function.

But the issue here is that in none of the bridgeIn, bridgeToRoot or the mint functions the _recipient address is not checked for address(0). Hence if the address(0) is passed in as the _recipient address to the RootBridgeAgent.bridgeIn function (address(0) is the default value), the _hTokens transferred to the _recipient address (address(0)) will be permenantly lost.

Hence it is recommneded to perform the address(0) check inside the ``IPort.bridgeToRoot` function as an input validation check.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L351 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L377-L378 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L277-L291

7. KEY DATA TYPE AND NAMING MISMATCH IN THE MAPPING DECLARATION OF THE RootPort CONTRACT

In the RootPort.sol contract there are four mappings declared on hTokens. They are as follows:

/// @notice ChainId -> Local Address -> Global Address
mapping(address chainId => mapping(uint256 localAddress => address globalAddress)) public getGlobalTokenFromLocal;

/// @notice ChainId -> Global Address -> Local Address
mapping(address chainId => mapping(uint256 globalAddress => address localAddress)) public getLocalTokenFromGlobal;

/// @notice ChainId -> Underlying Address -> Local Address
mapping(address chainId => mapping(uint256 underlyingAddress => address localAddress)) public
    getLocalTokenFromUnderlying;

/// @notice Mapping from Local Address to Underlying Address.
mapping(address chainId => mapping(uint256 localAddress => address underlyingAddress)) public
    getUnderlyingTokenFromLocal;

As it is evident in each of the above mappings there is a mismatch between chainId and localAddress names used.

These mappings declare that the first key as an address (which is named chainId but it's actually an address), and the second key as a uint256 (which is named localAddress, which is misleading because it's a uint256, not an address). This can be confusing to both developers and auditors and can lead to serious vulenarabilities if the naming mismatch is not understood properly.

Hence it is recommended to correct the above key data type and naming mismatch, and update the mapping declaration as shown below:

/// @notice Local Address -> ChainId -> Global Address
mapping(address localAddress => mapping(uint256 chainId => address globalAddress)) public getGlobalTokenFromLocal;

/// @notice Global Address -> ChainId -> Local Address
mapping(address globalAddress => mapping(uint256 chainId => address localAddress)) public getLocalTokenFromGlobal;

/// @notice Underlying Address -> ChainId -> Local Address
mapping(address underlyingAddress => mapping(uint256 chainId => address localAddress)) public
    getLocalTokenFromUnderlying;

/// @notice Mapping from Local Address to Underlying Address.
mapping(address localAddress => mapping(uint256 chainId => address underlyingAddress)) public
    getUnderlyingTokenFromLocal;

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L89-L101

8. ERC20hTokenRootFactory CONTRACT DOES NOT HAVE FUNCTIONALITY TO REMOVE UNWANTED hTokens IF THE NEED ARISES IN THE FUTURE

The ERC20hTokenRootFactory contract is a factory contract used to create new hTokens. The new hTokens are create using the createToken function and the newly created hTokens are added to the hTokens array as shown below:

    newToken = new ERC20hTokenRoot(
        localChainId,
        address(this),
        rootPortAddress,
        _name,
        _symbol,
        _decimals
    );
    hTokens.push(newToken);

The issue here is that even if the hTokens are added to the hTokens array there is no mechanism to remove the hTokens if the need arises in the future. As a result the hTokens array will keep on growing.

Since there is a ERC20hTokenRootFactory.getHTokens function to retrieve the hTokens array, the retreiver could be using the hTokens array to loop through the array to execute a different logic. If the hTokens array is extremely large then the retriever will get transaction out-of-gas exception. Hence the hTokens array returned by the ERC20hTokenRootFactory.getHTokens function will not be useful for any other logic execution.

Hence it is recommended to add a functionality remove the unwanted global hTokens from the hTokens array in the ERC20hTokenRootFactory contract and update the corresponding token mappings (Eg : getGlobalTokenFromLocal) in the RootPort contract.

https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ERC20hTokenRootFactory.sol#L81-L89 https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ERC20hTokenRootFactory.sol#L63-L65 https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ERC20hTokenBranchFactory.sol#L72 https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ERC20hTokenBranchFactory.sol#L87-L89

9. address(0) CHECK IS NOT PERFORMED ON THE _newBranchBridgeAgent ADDRESS IN THE RootPort.syncBranchBridgeAgentWithRoot FUNCTION

The RootPort.syncBranchBridgeAgentWithRoot function is used to sync the branch bridge agent for a given _branchChainId with the root bridge agent. The function calls the IBridgeAgent.syncBranchBridgeAgent function to perform the _newBranchBridgeAgent update.

But the issue is there is no address(0) check performed on the _newBranchBridgeAgent address for which the branch bridge agent will be set to.

Hence it is recommended to perform the address(0) check on the _newBranchBridgeAgent value inside the RootPort.syncBranchBridgeAgentWithRoot to ensure address(0) is not as the branch bridge agent address for the given _branchChainId in the respective _rootBridgeAgent.

Following line of code can be added to the RootPort.syncBranchBridgeAgentWithRoot function before the BridgeAgent.syncBranchBridgeAgent is called.

require(_newBranchBridgeAgent != address(0), "Can not be address(0)");

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L404 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1181-L1182

10. RETURN BOOLEAN VALUE OF approve TRANSACTION IS NOT CHECKED FOR SUCCESS IN THE BaseBranchRouter._transferAndApproveToken FUNCTION

The BaseBranchRouter._transferAndApproveToken is an internal function used to transfer tokens to the Branch Router during callOutAndBridge transaction. During the _transferAndApproveToken function execution both the local branch tokens (_hToken) and underlying tokens (_token) are transferred to the BaseBranchRouter contract.

Then both the _hToken and the _token are approved by the BaseBranchRouter contract to the _localPortAddress contract to be stored in the port. It is shown below:

        ERC20(_hToken).approve(_localPortAddress, _amount - _deposit);

        ERC20(_token).approve(_localPortAddress, _deposit);              

This issue here is certain ERC20 tokens communicate the approve transaction failure by returning a boolean false value. Since the above lines of code does not check the return boolean value of the approve function it could lead to failed transactions being accepted as succesful without approving anything.

Hence it is recommneded to check the return boolean value of the approve transaction and revert if the success value of the returned boolean is false.

Note : In the automated report one instance (BaseBranchRouter.sol#L175) is mentioned. But There is another instance of this issue in BaseBranchRouter.sol#L168. As a result included this as a quality assurance vulnerabilitiy.

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

11. THE _refundee CAN UNFAIRLY PROFIT FROM THE REFUNDED GAS SINCE address(this).balance IS USED TO CALCULATE THE BALANCE GAS AMOUNT TO BE SENT TO THE _refundee

The RootBridgeAgent contract and BranchBridgeAgent contracts use address(this).balance to calculate the existing balance of the contract to pass as the gas amount during execution of the RootBridgeAgent._execute, RootBridgeAgent._performFallbackCall functions and while calling the _performRetrySettlementCall function.

The issue is, if there is native Eth transferred to RootBridgeAgent or BranchBridgeAgent by another contract or an EOA by mistake those funds will also be transferred to the _refundee address of the respective transaction. Hence the _refundee gets an undue advantage and will profit with extra native Eth from the mistake of another individual.

Hence it is recommended to calculate the eth balance before and after the function call which transfers native eth to either RootBridgeAgent or BranchBridgeAgent contracts and then use the actually transferred eth amount to execute the cross-chain transaction. This can be done by declaring a state variable to keep track of the current eth balance of the contract at all times. And then use that variable to calculate the actually transferred eth amount to the contract. This will refund the correct amount of gas to the _refundee address if excessive gas remained after the transaction execution.

Once above change is made it is recommneded to add a withdraw function to withdraw the locked eth amount from the RootBridgeAgent contract and BranchBridgeAgent contracts. This will ensure fair operation of the system for all users.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L695 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L754 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L779 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L940 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L717 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L737 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L787

12. BranchPort.initialize FUNCTION CAN BE CALLED MULTIPLE TIMES BY THE owner THUS MAKING THE CONTRACT VULNERABLE TO CENTRALIZATION AND SINGLE POINT OF FAILURE

The BranchPort.initialize function is onlyOwner controlled function which is used to initialize the coreBranchRouterAddress and isBridgeAgentFactory[_bridgeAgentFactory] state variables as shown below:

    coreBranchRouterAddress = _coreBranchRouter;
    isBridgeAgentFactory[_bridgeAgentFactory] = true;
    bridgeAgentFactories.push(_bridgeAgentFactory);

The issue here is that this function can be called by the owner any number of times thus changing the coreBranchRouterAddress address as he wants. This is not the expected behaviour of the initialize function which is expected to be called only once to initialze the states of the smart contract. Due to the above behaviour of the initialize function the owner is getting a greater control over the BranchPort contract which makes the contract vulnerable to single point of failure.

Hence it is recommended to import the openzeppelin Initializable.sol contract and add the initializer modifier to the BranchPort.initialize function so it can be called only once by the owner to initialize the smart contract.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L122

13. BranchPort._checkTimeLimit TRANSACTION REVERT DUE TO ARITHMETIC UNDERFLOW IS NOT CORRECTLY HANDLED

The BranchPort._checkTimeLimit function is used to check if a Port Strategy has reached its daily management limit. The remaining daily limit is updated at the end of the _checkTimeLimit function as shown below:

    strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;

But there is no check performed to ensure dailyLimit > _amount. Hence if the dailyLimit < _amount transaction could revert silently. Hence it is recommended to add the following require statement before the strategyDailyLimitRemaining[msg.sender][_token] is updated.

    require(dailyLimit > _amount, "Daily limit has exceeded");
    strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L493

14. DISCREPENCY BETWEEN Natspect COMMENTS AND ACTUAL LOGIC IMPLEMENTATION IN THE hToken Address PARSING IN THE RootBridgeAgentExecutor._bridgeInMultiple FUNCTION

The RootBridgeAgentExecutor._bridgeInMultiple function parses the input bytes array _dParams into the respective token details. The Natspect comment on the parsing of the hToken address is given as below:

 *         1. PARAMS_TKN_START [hToken address has no offset from token information start]

In the Natspec comment is says there is no offset from the token information start. But as it is evident from the logic implementation there is an offset of ADDRESS_END_OFFSET byte amount as shown below:

    _dParams[
        PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * i) + ADDRESS_END_OFFSET:
        PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * currentIterationOffset)
    ]

This discrepency between the Natspec comment and actual logic implementation could be confusing to both developers and auditors.

Hence the Natspec comment for the hToken address should be corrected to mention there is an offset of ADDRESS_END_OFFSET from token information start.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L262 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L286-L297

15. ERRORNEOUS NATSPEC COMMENT IN THE BranchBridgeAgent._performCall FUNCTION

The BranchBridgeAgent._performCall function is used to perform calls to LayerZero messaging layer Endpoint for cross-chain messaging. _gParams is passed in as a GasParams struct which includes the LayerZero gas information. The Natspec comments for the _gParams input parameter is given as shown below:

 *   @param _gParams LayerZero gas information. (_gasLimit,_remoteBranchExecutionGas,_nativeTokenRecipientOnDstChain)

But the _gParams is a variable of type GasParams struct and GasParams struct does not has a parameter named _nativeTokenRecipientOnDstChain within its scope. Hence above Natspec comments is errorneous and should be corrected as follows:

 *   @param _gParams LayerZero gas information. (_gasLimit,_remoteBranchExecutionGas)

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

16. TYPOS IN THE NATSPEC COMMENTS SHOULD BE CORRECTED

 *         `Naive` assets are deposited and hTokens are bridgedOut.

The above Natspec comment should be corrected as follows:

 *         `Native` assets are deposited and hTokens are bridgedOut.

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

/// @notice `Deposit` nonce used for identifying transaction.

The above Natspec comment should be corrected as follows:

/// @notice `Settlement` nonce used for identifying transaction.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L74

 *     @param _localRouterAddress Local `Port` Address. 

The above Natspec comment should be corrected as follows:

 *     @param _localRouterAddress Local `Router` Address. 

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L103

 * @notice Internal function performs call to Layerzero `Enpoint` Contract for cross-chain messaging.

The above Natspec comment should be corrected as follows:

 * @notice Internal function performs call to Layerzero `Endpoint` Contract for cross-chain messaging.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L934

/// @notice `Deposit` nonce used for identifying transaction.

The above Natspec comment should be corrected as follows:

/// @notice `Settlement` nonce used for identifying transaction.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L74

1. REDUNDANT CONDITIONAL CHECK IN THE BranchBridgeAgent.redeemDeposit CAN BE OMITTED TO SAVE GAS

The BranchBridgeAgent.redeemDeposit function is used to redeem the deposit by a given msg.sender. The redeemDeposit function performs the following input validation checks using the require statements.

    if (deposit.owner == address(0)) revert DepositRedeemUnavailable(); 
    if (deposit.owner != msg.sender) revert NotDepositOwner(); 

As it is seen above the deposit.owner == address(0) conditional checks is redundant since it is anyway verified that the deposit.owner is not a zero address in the conditional check which follows it.

If the deposit.owner == address(0) then deposit.owner != msg.sender is also true since the msg.sender can not be address zero. Hence it is recommended to remove the redundant deposit.owner == address(0) conditional check to save gas. The modified code is shown below:

    if (deposit.status == STATUS_SUCCESS) revert DepositRedeemUnavailable();
    if (deposit.owner != msg.sender) revert NotDepositOwner(); 

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L439-L441

2. delete CAN BE USED TO OBTAIN A GAS REFUND INSTEAD OF RESETTING TO ADDRESS(0)

The BranchBridgeAgent.redeemDeposit function is used to redeem the deposit by a given msg.sender. Inside the redeemDeposit function the deposit.owner is assigned the address(0) in order to zero out the owner as shown below:

    deposit.owner = address(0); 

But the delete can be used to obtain a gas refund instead of assigning to address(0). Hence the modified line of code is shown below:

    delete deposit.owner;

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

3. REDUNDANT ADD ARITHMETIC OPERATION CAN BE REPLACED TO SAVE GAS

The BranchBridgeAgentExecutor.executeWithSettlementMultiple function is used to execute a cross-chain request with multiple settlements. In the function execution if the _payload.length > settlementEndOffset condition is satisfied (which means there is calldata to be executed) executeSettlementMultiple function of the Router contract is called as shown below:

        IRouter(_router).executeSettlementMultiple{value: msg.value}(
            _payload[PARAMS_END_SIGNED_OFFSET + assetsOffset:], sParams
        );

The bytes range of the _payload is determined as PARAMS_END_SIGNED_OFFSET + assetsOffset:. But the arithmetic addition used in the above range calculation can be replaced by using the settlementEndOffset value since the settlementEndOffset == PARAMS_END_SIGNED_OFFSET + assetsOffset. Hence this will save gas on one + arithmetic operation. Hence the above code snippet can be modified as shown below:

        IRouter(_router).executeSettlementMultiple{value: msg.value}(
            _payload[settlementEndOffset:], sParams
        );

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgentExecutor.sol#L121-L123

4. ADDRESS VALIDATION CHECK CAN BE PERFORMED AT THE BEGINNING OF THE FUNCTION EXECUTION TO SAVE GAS INCASE OF TRANSACTION REVERT

The RootBridgeAgent._performRetrySettlementCall function performs call to Layer Zero Endpoint Contract for cross-chain messaging. During the function execution there is conditional check to ensure that the remoteBridgeAgent is a valid address and is not address(0) as shown below:

    address callee = getBranchBridgeAgent[_dstChainId];

    // Check if valid destination
    if (callee == address(0)) revert UnrecognizedBridgeAgent(); 

But the problem here is that this conditional check is performed after the payload computation for the remote call for either single asset settlement or multiple asset settlement. Hence if the callee == address(0) condition results in true the transaction will revert thus wasting the gas consumed on the payload computation.

Hence it is recommended to perform the callee == address(0) conditional check at the beggining of the RootBridgeAgent._performRetrySettlementCall execution thus saving gas if the transaction reverts due to the callee == address(0) condition resulting in false.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L906-L910

5. REDUNDANT ARITHMETIC OPERATIONS IN THE RootBridgeAgentExecutor.executeWithDepositMultiple CAN BE OMITTED TO SAVE GAS

The RootBridgeAgentExecutor.executeWithDepositMultiple function is used to execute a remote call with multiple deposits. The encoded data payload is passed to the executeWithDepositMultiple as _payload and the byte array is sliced to obtain the required portion of the _payload to execute the remote call.

The following calculation is performed to calculate the ending index of the _payload which represents the deposit details of the assets.

PARAMS_END_OFFSET + uint256(uint8(bytes1(_payload[PARAMS_START]))) * PARAMS_TKN_SET_SIZE_MULTIPLE

But the issue is this calculation is performed three times with in the executeWithDepositMultiple function redundantly.

Other two locations are as follows:

    if (length > PARAMS_END_OFFSET + (numOfAssets * PARAMS_TKN_SET_SIZE_MULTIPLE)) {

    IRouter(_router).executeDepositMultiple{value: msg.value}(
        _payload[PARAMS_END_OFFSET + uint256(numOfAssets) * PARAMS_TKN_SET_SIZE_MULTIPLE:], dParams, _srcChainId
    );

Hence it is recommended to perform the above mentioned calculation and assign the result into a memory variable and read the value from the memory to other two locations where the above calculation is used. This will save gas which is spent on redundant arithmetic operations to calculation the above mentioned bytes array index.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L125 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L134 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L136-L138 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L210-L214 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L220-L222 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L226-L229

6. THERE IS NO EQUALITY CHECK ON THE ARRAY LENGTHS IN THE BaseBranchRouter.callOutAndBridgeMultiple FUNCTION WHICH COULD LEAD TO GAS WASTAGE IN THE EVENT OF A REVERT

The BaseBranchRouter.callOutAndBridgeMultiple function is used to bridge in multiple tokens to the root. The function calls the _transferAndApproveMultipleTokens internal function which is used to transfer multiple tokens to the BaseBranchRouter contract and approve the tokens to the _localPortAddress contract. This is performed by iterating through a for loop till _hTokens.length is reached.

The issue here is if there is a length mismatch in any of the _hTokens, _tokens, _amounts or _deposits the transaction will revert inside the for loop due to index out of bound execption thus wasting the user's gas spent on the computations performed till that point.

Hence it is recommended to perform an array length equality check for the _hTokens, _tokens, _amounts and _deposits arrays inside the BaseBranchRouter.callOutAndBridgeMultiple function, before _transferAndApproveMultipleTokens function is called. Hence if there is an array length mismath then the transaction will revert at the very beginning of its execution without wasting gas on further computation.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BaseBranchRouter.sol#L104-L110 https://github.com/code-423n4/2023-09-maia/blob/main/src/BaseBranchRouter.sol#L192-L198

7. deposit.status IS NOT CHECKED AT THE BEGINNING OF THE BranchBridgeAgent.retryDeposit FUNCTION WHICH ALLOWS THE retryDeposit TO BE CALLED FOR THE SECOND TIME THUS WASTING GAS

The BranchBridgeAgent.retryDeposit function is used to retry the deposit transaction when the transaction is failed before. In the execution of the BranchBridgeAgent.retryDeposit the deposit.status will be updated to STATUS_SUCCESS as shown below:

    deposit.status = STATUS_SUCCESS;

But the issue here is the deposit.status is not checked for STATUS_SUCCESS at the beginning of the retryDeposit function which would allow a deposit.owner to mistakenly call the retryDeposit on the _depositNonce for the second time even after succesfully executing it the first time. As a result the second call will only revert in the RootBridgeAgent.lzReceiveNonBlocking function when _payload[0] == 0x03 occurs and the following condition occurs:

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

But the gas spent on the second retryDeposit transaction till it reverts after reaching the RootBridgeAgent.lzReceiveNonBlocking revert conditon will be lost. But this gas wastage can be avoided by performing the status check on the deposit.status value at the beginning of the BranchBridgeAgent.retryDeposit function as shown below:

require(deposit.status != STATUS_SUCCESS, "Deposit already executed");

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L415 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L518-L520

8. ARITHMETIC OPERATIONS CAN BE unchecked DUE TO PREVIOUS CONDITIONAL CHECKS, TO SAVE GAS

The BranchPort.replenishReserves function is used to replenish the minimum reserves required for a particular strategy token. The strategy tokens will be withdrawn from the specific _strategy to replenish the minimum reserve amount.

getPortStrategyTokenDebt and getStrategyTokenDebt mappings are updated accordingly by the replenished strategy token amount as shown below:

    // Update Port Strategy Token Debt
    getPortStrategyTokenDebt[_strategy][_token] = portStrategyTokenDebt - amountToWithdraw;
    // Update Strategy Token Global Debt
    getStrategyTokenDebt[_token] = strategyTokenDebt - amountToWithdraw;

Both of the above arithmetic operations can not underflow due to previous conditonal check which is given below:

    uint256 amountToWithdraw = portStrategyTokenDebt < reservesLacking ? portStrategyTokenDebt : reservesLacking;

Hence it is recommended to unchecked the getPortStrategyTokenDebt and getStrategyTokenDebt updates to save gas.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L201-L207

9. REDUNDANT require STATEMENT CAN BE REMOVED TO SAVE GAS IN THE BranchPort.setCoreRouter FUNCTION

The BranchPort.setCoreRouter is used to set a new coreBranchRouterAddress address as shown below:

function setCoreRouter(address _newCoreRouter) external override requiresCoreRouter {
    require(coreBranchRouterAddress != address(0), "CoreRouter address is zero");
    require(_newCoreRouter != address(0), "New CoreRouter address is zero");
    coreBranchRouterAddress = _newCoreRouter;
}

There is a redundant check of coreBranchRouterAddress != address(0) check which is redundant and hence can be removed to save gas. Since the setCoreRouter function is controlled by the requiresCoreRouter modifier, only the coreBranchRouterAddress can call the setCoreRouter function. Hence the coreBranchRouterAddress can never be address(0).

Hence the coreBranchRouterAddress != address(0) require statement check is redundant and should be removed to save gas.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L331-L335