Skip to content
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

Non-whitelisted users can burn UStb and redeem collateral during WHITELIST_ENABLED state #13

Open
c4-bot-3 opened this issue Nov 11, 2024 · 8 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden M-02 primary issue Highest quality submission among a set of duplicates 🤖_03_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-3
Copy link
Contributor

c4-bot-3 commented Nov 11, 2024

Lines of code

https://github.com/code-423n4/2024-11-ethena-labs/blob/e93ee09b10f900bd3be385f392c80920898bf53e/contracts/ustb/UStb.sol#L189

Vulnerability details

Finding description and impact

Non-whitelisted users can redeem collateral tokens and burn their UStb even when whitelist mode has been enabled on UStb contract. This breaks the main invariant mentioned in the README.

Proof of Concept

The below code block is from the _beforeTokenTransfer() function(), which is called at the beginning of the ERC20 _burn() internal function. When the transferState is WHITELIST_ENABLED, it should only allow whitelisted users to burn their UStb as mentioned under the main invariants in the README. But since the from address is not checked to have the WHITELISTED_ROLE as well, the call goes through.

File: UStb.sol
193:         } else if (transferState == TransferState.WHITELIST_ENABLED) {
194:            
195:             if (hasRole(MINTER_CONTRACT, msg.sender) && !hasRole(BLACKLISTED_ROLE, from) && to == address(0)) {

Recommended mitigation steps

Add hasRole(WHITELISTED_ROLE, from) in the check.

@c4-bot-3 c4-bot-3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 11, 2024
c4-bot-9 added a commit that referenced this issue Nov 11, 2024
@c4-bot-13 c4-bot-13 added the 🤖_03_group AI based duplicate group recommendation label Nov 11, 2024
@c4-judge
Copy link

0xEVom marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Nov 12, 2024
@iethena
Copy link

iethena commented Nov 15, 2024

The Redeemer has a special role in the protocol and it is seen as a non-issue that UStb can be redeemed from a non-whitelisted address while whitelist mode is enabled. As specified in the Overview the redeem order is determined by and off-chain RFQ system, in which case the Redeemer will not provide a redemption quote if they chose not to.

For clarity, a non-whitelisted user cannot redeem collateral without the involvement of the Redeemer, as it is the Redeemer who submits the settlement transaction on-chain. We therefore argue that the likelihood of this happening is low. In addition, the impact to the protocol is low as the collateralization ratio of the minting contract would not be impacted and other users' positions are not impacted. Although this finding is informationally correct, we view the severity to be based on the fact that a non-whitelisted address can initiate a redemption without the involvement of a trusted party within the protocol which is not the case.

We suggest marking this finding as Low severity.

@iethena iethena added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 15, 2024
@0xEVom
Copy link

0xEVom commented Nov 18, 2024

One of the main invariants here was:

Only whitelisted user can send/receive/burn UStb tokens in a WHITELIST_ENABLED transfer state.

The warden could not have known whether the off-chain Redeemer would only submit transactions for whitelisted addresses. Besides, the WHITELIST_MANAGER and the REDEEMER being different entities means there can always be race conditions on address (un-)whitelisting and redemption submission.

If the contract must enforce the invariant (which seems to not necessarily be the case), this must be done onchain. But for the purpose of the contest, Medium is appropriate.

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 18, 2024
@c4-judge
Copy link

0xEVom marked the issue as satisfactory

@c4-judge
Copy link

0xEVom marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Nov 18, 2024
@KupiaSecAdmin
Copy link

KupiaSecAdmin commented Nov 18, 2024

@0xEVom! Thanks for your judging. I think this issue should be seen as QA based on the following C4 rule.

Centralization Risk

  • Direct misuse of privileges shall be submitted in the QA report.

Redeemer has a trusted role, that's why redeeming collateral by buring USTB for a unwhitlisted user in the WHITELIST_ENABLED case is a direct misuse of privileges.

In README.md:

All trusted roles in the protocol:

| Role              | Description                                                                        |
|-------------------|------------------------------------------------------------------------------------|
| DEFAULT_ADMIN     | can assign roles to themselves or others and has the ability to perform any action |
| MINTER            | has the ability to call the mint function in UStb minting contract                 |
| REDEEMER   @>    | has the ability to call the redeem function in UStb minting contract               |  
| GATEKEEPER        | has the ability to disable minting and redeeming                                   |
| BLACKLIST_MANAGER | has the ability to blacklist addresses                                             |
| WHITELIST_MANAGER | has the ability to whitelist addresses                                             |
| COLLATERAL_MANAGER| has the ability to withdraw collateral to custodians                               |

I submitted this issue for QA as a consequence.

@0xEVom
Copy link

0xEVom commented Nov 20, 2024

The redeemer is not expected to enforce the whitelist - that is the WHITELIST_MANAGER, and the smart contract's, responsibility.

@0xEVom
Copy link

0xEVom commented Nov 20, 2024

Would have been happy to upgrade an L finding to a duplicate, but your L-12 only points out the root cause in #14.

@C4-Staff C4-Staff added the M-02 label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden M-02 primary issue Highest quality submission among a set of duplicates 🤖_03_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

8 participants