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

The burn function in rUSDY allows the caller to benefit by transferring the burned shares to themselves rather than burning them. #270

Closed
code423n4 opened this issue Sep 7, 2023 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-85 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L680

Vulnerability details

Impact

The caller can burn rUSDY from any account and gain the underlying USDY shares for themselves. This could be used maliciously to steal USDY shares.It allows the caller to collect free shares at the expense of other holders, by artificially increasing the rUSDY price.

Proof of Concept

The key part is this line: https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L680

This transfers the burnt shares to msg.sender rather than burning them.

This allows the caller of the burn function to get free shares. For example:

  • Alice has 100 rUSDY tokens, backed by 10000 shares
  • Bob has the BURNER_ROLE and calls burn on Alice's account for 50 rUSDY
  • This burns 5000 of Alice's shares
  • But instead of being burned, the 5000 shares are transferred to Bob
  • Now Bob has 5000 free shares that he did not pay for
  • Alice lost 50 rUSDY tokens and 5000 shares, but the shares did not get burned, they got transferred to Bob

This exploits the rebasing mechanic of rUSDY. When shares get burned, the rUSDY price increases for remaining holders because totalSupply decreases but pooled USDY remains the same.

So this vulnerability allows the caller to collect free shares at the expense of other holders, by artificially increasing the rUSDY price.

Tools Used

Manual

Recommended Mitigation Steps

The burn function should actually burn the shares instead of transferring them or the burn() function could be changed to send the shares to an authorized share burner address instead of the caller

Assessed type

Other

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 7, 2023
code423n4 added a commit that referenced this issue Sep 7, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #85

@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 8, 2023
@c4-judge
Copy link
Contributor

kirk-baird changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 19, 2023
@c4-judge
Copy link
Contributor

kirk-baird marked the issue as grade-c

@c4-judge c4-judge reopened this Sep 26, 2023
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Sep 26, 2023
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by kirk-baird

@c4-judge
Copy link
Contributor

kirk-baird changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 26, 2023
@C4-Staff C4-Staff closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-85 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants