Maia DAO - Ulysses - Arz's results

Lines of code

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

Vulnerability details

Ecosystem tokens are tokens that dont have an underlying token address in any branch and only the global representation exists. The governance adds them by calling addEcosystemToken() where the _ecoTokenGlobalAddress will be the Maia or Hermes token as the sponsor mentioned or any other tokens that could be added in the future.

The problem is that anyone can create a hToken where the underlying asset is the ecosystem token and then this mapping will get updated in setAddresses():

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L252

252:  getLocalTokenFromUnderlying[_underlyingAddress][_srcChainId] = _localAddress;

The _underlyingAddress equals to the _ecoTokenGlobalAddress and when the admin calls addEcosystemToken() then it will revert because of this check that is incorrect:

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

493:   if (getLocalTokenFromUnderlying[_ecoTokenGlobalAddress][localChainId] != address(0)) {
494:       revert AlreadyAddedEcosystemToken();
495:   }

getLocalTokenFromUnderlying[_ecoTokenGlobalAddress] wont be a zero address because it was set when the attacker added the hToken.

This check here is redundant and even if someone creates a hToken where the underlying asset will be the ecosystem token then it will not be tied to the ecosystem token in any way because it has a different global address and a different local address.

Impact

The governance will fail to add the ecosystem tokens and they will not work with this part of Ulysses because an attacker can easily create a hToken before the ecosystem token is set.

Proof of Concept

Add this to RootTest.t.sol

function testAddEcosystemTokenAttack() public {
        //arbitrumMockToken will be the Maia or Hermes token
        hevm.deal(address(this), 1 ether);

        //Attacker adds the Maia or Hermes token
        arbitrumCoreRouter.addLocalToken{value: 0.0005 ether}(
            address(arbitrumMockToken), GasParams(0.5 ether, 0.5 ether)
        );

        newArbitrumAssetGlobalAddress =
            RootPort(rootPort).getLocalTokenFromUnderlying(address(arbitrumMockToken), rootChainId);

        require(
            RootPort(rootPort).getGlobalTokenFromLocal(address(newArbitrumAssetGlobalAddress), rootChainId)
                == address(newArbitrumAssetGlobalAddress),
            "Token should be added"
        );
        require(
            RootPort(rootPort).getLocalTokenFromGlobal(newArbitrumAssetGlobalAddress, rootChainId)
                == address(newArbitrumAssetGlobalAddress),
            "Token should be added"
        );
        require(
            RootPort(rootPort).getUnderlyingTokenFromLocal(address(newArbitrumAssetGlobalAddress), rootChainId)
                == address(arbitrumMockToken),
            "Token should be added"
        );

        //The admin will then fail to add an ecosystem token because the tx reverts
        rootPort.addEcosystemToken(address(arbitrumMockToken));

    }

As you can see here the tx reverts because of that check and the admin will fail to add the ecosystem token

Tools Used

Foundry

Recommended Mitigation Steps

Remove this check as its redundant, setting the ecosystem tokens when initializing is not the best solution because other tokens can be added in the future.

if (getLocalTokenFromUnderlying[_ecoTokenGlobalAddress][localChainId] != address(0)) {
     revert AlreadyAddedEcosystemToken();
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L749 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L712

Vulnerability details

If a tx on the destination chain calls back the chain from where the transaction was initiated by the user, the first transaction on the source chain needs to "airdrop" gas to the destination chain so it is able to call back the source chain.

The problem is that when the transaction on the destination chain fails and there is no fallback, gas that was sent from the source chain isnt refunded and it will be stuck on the destinations chain bridge agent.

Not every operation type allows you to use the fallback like system requests or operations that are not signed so if the transaction fails then the airdropped gas will be stuck and it will be used by other users.

Example:

1. Two users want to add the same global token and they both call addGlobalToken() on the branch chain. When addGlobalToken() is used then you need to airdrop the gas for 2 transactions
2. The transaction that was first caught by LayerZero will be executed succesfuly on the destination chain
3. However the second transaction will fail because the token is already added and the _execute() without fallback was used
4. The gas that was sent to the root chain will be locked in the RootBridgeAgent and it can then be used by other users

Please not that this doesnt only have to be a system request it can also be a non-signed operation or when fallback isnt toggled.

Impact

Ether will be stuck in the BridgeAgent contract and the user that called the transaction wont be able to get it back and it will then be taken by other users when they for example execute _performFallbackCall() where the LayerZero will refund them the contract balance.

Proof of Concept

Add this to RootForkTest.t.sol, the poc shows that if the second transaction fails then funds will be stuck in the RootBridgeAgent.

function testAddGlobalTokenFails() public {

        //Add Local Token from Avax
        testAddLocalToken();

        console2.log("BALANCE OF ROOT BRIDGE AGENT BEFORE", address(coreRootBridgeAgent).balance);

        //the tx fails on the second iteration, because token already added
        for(uint256 i; i < 2; i++) {
            switchToLzChain(avaxChainId);

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

            GasParams[3] memory gasParams =
                [GasParams(15_000_000, 0.1 ether), GasParams(2_000_000, 3 ether), GasParams(200_000, 0)];

            avaxCoreRouter.addGlobalToken{value: 1000 ether}(newAvaxAssetGlobalAddress, ftmChainId, gasParams);

            //Switch Chain and Execute Incoming Packets
            switchToLzChain(rootChainId);
            //Switch Chain and Execute Incoming Packets
            switchToLzChain(ftmChainId);
            //Switch Chain and Execute Incoming Packets
            switchToLzChain(rootChainId);

            newAvaxAssetFtmLocalToken = rootPort.getLocalTokenFromGlobal(newAvaxAssetGlobalAddress, ftmChainId);
        }   
        console2.log("BALANCE OF ROOT BRIDGE AGENT AFTER", address(coreRootBridgeAgent).balance);
    }

Tools Used

Foundry

Recommended Mitigation Steps

The first _execute function should be refactored so that when the call to bridgeAgentExecutorAddress fails, the tx shouldnt be reverted but instead gas will be refunded and the executionState will be set back to 0 as that was the only state change in the BridgeAgent.

If the localChainId == _srcChainId then the transaction should be reverted as we are not using LayerZero.

function _execute(uint256 _depositNonce, bytes memory _calldata, uint16 _srcChainId, address _refundee) private {
        //Update tx state as executed
        executionState[_srcChainId][_depositNonce] = STATUS_DONE;

        //Try to execute the remote request
        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

        // No fallback is requested revert allowing for retry.
        if (!success) {
            if(localChainId == _srcChainId) revert ExecutionFailure();

            //REFUND GAS
            (bool success2,) = _refundee.call{value: address(this).balance}("");
            require(success2);

            //REVERT STATE CHANGES, OTHER STATE CHANGES IN bridgeAgentExecutorAddress ARE REVERTED WHEN THE TX FAILS THERE
            executionState[_srcChainId][_depositNonce] = STATUS_READY;
        }
    }

Assessed type

Other