-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The governance will fail to add an ecosystem token if someone creates a hToken that uses that ecosystem token #881
Comments
0xA5DF marked the issue as primary issue |
0xA5DF marked the issue as sufficient quality report |
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. |
0xBugsy (sponsor) disputed |
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). |
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. |
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. |
alcueca marked the issue as satisfactory |
alcueca marked the issue as selected for report |
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 |
Hi @stalinMacias , the attacker is not calling rootPort's onlyOwner setters, but from the arbitrumCoreRouter (a branch chain). By doing that he can make As for the PoC, you can add the 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 |
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. |
0xLightt (sponsor) confirmed |
0xLightt marked the issue as disagree with severity |
Updated review and feedback to reflect our opinion on this issue. |
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.
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. |
Addressed at Maia-DAO/2023-09-maia-remediations@e715c21 |
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
The
_underlyingAddress
equals to the_ecoTokenGlobalAddress
and when the admin callsaddEcosystemToken()
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
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
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.
Assessed type
DoS
The text was updated successfully, but these errors were encountered: