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 depositer can break Vault share distributions #307

Closed
code423n4 opened this issue Nov 28, 2022 · 3 comments
Closed

First depositer can break Vault share distributions #307

code423n4 opened this issue Nov 28, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-275 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

code423n4 commented Nov 28, 2022

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L164-L175

Vulnerability details

The calculation of exchange rate for shares in PirexERC4626 Vault is done by dividing the total supply of shares by the totalAssets of the vault. The first depositor can mint a very small number of shares, then donate to the vault to manipulate the share price. When subsequent depositors deposit, they will lose value due to precision loss. This is a common attack vector for almost all shares based liquidity pool contracts using ERC4626.

Impact

First depositor can manipulate shares from later users; later users will not get equivalent shares when converting their underlying asset.

Proof of Concept

  1. Malicious user Alice can deposit() with 1 wei of asset token to get 1 wei of shares.
  2. Next, Alice sends 10000e18 -1 of asset tokens and inflate the price per share from 1 to 1e22.
  3. Subsequent depositor who deposits shares, eg 19999e18 of assets, will only receive 1 wei of shares token.
  4. Victim will lose 9999e18 if they redeem() right after deposit() due to precision loss.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156-L165

Relatable issue: https://github.com/sherlock-audit/2022-08-sentiment-judging#issue-h-1-a-malicious-early-userattacker-can-manipulate-the-ltokens-pricepershare-to-take-an-unfair-share-of-future-users-deposits

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimum amount of share tokens to be minted for the first minter or follow Uniswap V2 which mints 10,000 share first to balance liquidity.

https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L119-L124

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 28, 2022
code423n4 added a commit that referenced this issue Nov 28, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Dec 3, 2022

Picodes marked the issue as duplicate of #407

@c4-judge
Copy link
Contributor

c4-judge commented Jan 1, 2023

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 1, 2023
@C4-Staff
Copy link
Contributor

JeeberC4 marked the issue as duplicate of #275

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-275 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants