search

code-423n4 / 2023-09-ondo-findings

7 stars 5 forks source link

Lack of Post-Allowance Verification Before Token Minting #512

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L349

Vulnerability details

The code checks if the user (txn.sender) is allowed via the ALLOWLIST. If not, it sets the user's status to "allowed". However, after this step, there's no subsequent verification to ensure the user has indeed been added to the ALLOWLIST before minting tokens to them.

Impact

An unauthorized user might get tokens minted if there's a discrepancy in the setAccountStatus function or if there are other conditions that prevent the user from being added to the ALLOWLIST. This can lead to unauthorized minting, potentially diluting the token supply or allowing malicious actors to exploit the system.

Proof of Concept

Imagine Alice initiates a token minting process. The contract checks if Alice is on the ALLOWLIST. Let's assume she's not. The contract then tries to add Alice to the ALLOWLIST.

However, due to some unforeseen condition or a bug in setAccountStatus, Alice isn't successfully added to the ALLOWLIST. Despite this, the contract proceeds to mint tokens for Alice.

Now, Alice, who was initially not authorized and somehow failed to be added to the ALLOWLIST, still receives tokens. This scenario can be exploited by potential attackers to mint tokens without proper authorization.

Tools Used

Recommended Mitigation Steps

After setting the account status to allowed, a subsequent verification should be added to ensure the user is indeed allowed before minting the tokens to them.

Assessed type

Access Control

raymondfam commented 1 year ago

No issue here. If it's disallowed, BURNER_ROLE will simply burn the amount.

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

kirk-baird commented 1 year ago

This issue assumes there is a bug in setAccountStatus without providing a concrete example of how this bug can occur.

Marking it as invalid.

c4-judge commented 1 year ago

kirk-baird marked the issue as unsatisfactory: Invalid