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

First Deposit Can be frontrun to make amount minted round down to zero #85

Closed
c4-submissions opened this issue Nov 11, 2023 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-42 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 11, 2023

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L95-L110

Vulnerability details

Impact

First Deposit Can be frontrun to make amount minted round down to zero, which means the attacker gets 100% of the victims deposit contributed to the rsEth exchange rate.

Proof of Concept

When deposit is called when there are already pre-existing deposits, the conversion rate is totalETHInPool / rsEthSupply

When an intial deposit is sent to the pool, this can be front run by:

  1. Alice mints 1 rsEth
  2. Alice sends a large eth deposit directly into a contract that is accounted for in totalETHInPool such that totalETHInPool / rsEthSupply is greater than amount * lrtOracle.getAssetPrice(asset) of the victim's deposit
  3. The victims deposit goes in.
  4. totalETHInPool / rsEthSupply is greater than amount * lrtOracle.getAssetPrice(asset)
  5. Therefore the amount minted rounds down to zero.

Therefore zero shares are minted for the victim, and will result in a greater share of the pool for the attacker, since they still have 100% rsEth, and a boosted conversion rate, which still applies for future deposits. When withdrawals are implemented, this also means they can withdraw the entire victims deposit and whatever assets they sent directly to the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Use internal accounting which tracks deposits and withdrawals rather than balanceOf

Assessed type

Math

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 11, 2023
c4-submissions added a commit that referenced this issue Nov 11, 2023
@code4rena-admin code4rena-admin changed the title First rsEth Minter can be Frontrun with Inflation Attack Can Mint Far More RsEth Than One Should Through BalanceOf Manipulation Nov 13, 2023
@code4rena-admin code4rena-admin changed the title Can Mint Far More RsEth Than One Should Through BalanceOf Manipulation First Deposit Can be frontrun to make amount minted round down to zero Nov 13, 2023
@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 Nov 15, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #42

@c4-judge
Copy link
Contributor

c4-judge commented Dec 1, 2023

fatherGoose1 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-42 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants