The governance will fail to add an ecosystem token if someone creates a hToken that uses that ecosystem token

[c4-submissions]

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

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[0xBugsy]

If the token has not yet been added to the system a new one can simply be deployed. In addition this can be done in a multicall paired with the token deployment itself for added security. Removing this check would lead to added governance power / responsibility.

[c4-sponsor]

0xBugsy (sponsor) disputed

[alcueca]

Regardless of the mitigation, which might not be great, the issue is that if an ecosystem token is not set, an attacker can add it as an underlying of some other token and then it will not be possible to set it anymore as a global address (because the ecosystem token already is an underlying token).

[0xLightt]

The attacker would have to act between token deployment and adding it as an ecosystem token, only way for that to happen would need to come from a governance/setup mistake.

The check cannot be overridden if there is any deposits of that underlying or there would be funds lost. Mitigation could be improved by this, allow an ecosystem token to override a global token if it's total supply is zero. This way, any mistake can be circumvented without any redeployment (if the ecosystem token is not distributed yet) and our setup can be more flexible.

[alcueca]

Adding an ecosystem token immediately after creation is not obvious, or in the documentation. This is then a valid DoS attack, which you can mitigate with careful governance.

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[stalinMacias]

Hey @alcueca , I think this issue is more of a QA, based on the sponsor's comment "only way for that to happen would need to come from a governance/setup mistake. All of the functions that are able to update the state of ecosystem & global tokens have an access control that can only be called by governance, how can an attacker simply go ahead and set any token they wish as an ecosystemToken?

From the PoC, I can see the code is not running a prank to simulate the execution of the addLocalToken() to be made from an attacker, instead, the PoC executes that call as the owner of the contract, that's why it doesn't fail, but in production, attacker can't just simply gain access to the owner account and do that types of calls.

Could you revisit this issue again?

Also, @0xLightt , could you correct me if what I've just said is incorrect, thanks a lot for your time

[QiuhaoLi]

Hi @stalinMacias , the attacker is not calling rootPort's onlyOwner setters, but from the arbitrumCoreRouter (a branch chain). By doing that he can make _srcChainId of rootPort:setAddresses same as the localChainId of the root chain (Arbitrum).

As for the PoC, you can add the prank to simulate a better scenario, but it will still work:

function testAddEcosystemTokenAttack() public {

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

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

Seems like a good catch. CC: @alcueca

[peakbolt]

I have to agree with the sponsor that this issue is a governance error and is a good QA. The issue is more of a lack of proper documentation on setup and adding ecosystem token.

Otherwise, governance error issues like #345 would be valid if we go by the argument that the correct use is not obvious and not in the doc. Furthermore, #345 also explained that the error exists within the original test case, which means that the test case itself is demonstrating the error as a correct usage.

[c4-sponsor]

0xLightt (sponsor) confirmed

[c4-sponsor]

0xLightt marked the issue as disagree with severity

[0xLightt]

Updated review and feedback to reflect our opinion on this issue.

[alcueca]

Thanks for your feedback, but this stays as Medium.

Someone correct me if I'm wrong, but as an "ecosystem token" I understand something like MAIA. A token that is not necessarily used only for Ulysses. As such, I would expect that the creation of an ecosystem token be independent from the Ulysses deployment and management, unless explicitly set in the documentation and enforced somehow.

The attacker would have to act between token deployment and adding it as an ecosystem token, only way for that to happen would need to come from a governance/setup mistake.

My take here is that unless specifically stated in the docs, I see this as a very easy mistake to make. In other words, if this vulnerability wouldn't have been reported, why would MaiaDAO add ecosystem tokens to Ulysses in the same transaction that they are deployed?

The comparison to #345 is not reasonable. In #345 governance is required to enter non-sensical parameters into a call in order for the protocol to suffer. In this action, governance doesn't need to do anything obviously wrong.

[0xBugsy]

Addressed at Maia-DAO/2023-09-maia-remediations@e715c21