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 ERC4626 deposit can break share calculation #614

Closed
code423n4 opened this issue Jan 19, 2023 · 4 comments
Closed

First ERC4626 deposit can break share calculation #614

code423n4 opened this issue Jan 19, 2023 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-588 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L107-L113

Vulnerability details

Impact

ERC4626 vault share price can be maliciously inflated on the initial deposit, leading to the next depositor losing assets due to precision issues.
https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L107-L113
https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L105

Proof of Concept

The first depositor of an ERC4626 vault can maliciously manipulate the share price by depositing the lowest possible amount (1 wei) of liquidity and then artificially inflating ERC4626.totalAssets.

This can inflate the base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as a base and worst case, due to rounding down, if this malicious initial deposit front-run someone else depositing, this depositor will receive 0 shares and lost his deposited assets.

Given a vault with DAI as the underlying asset:

Alice (attacker) deposits initial liquidity of 1 wei DAI via deposit()
Alice receives 1e18 (1 wei) vault shares
Alice transfers 1 ether of DAI via transfer() to the vault to artificially inflate the asset balance without minting new shares. The asset balance is now 1 ether + 1 wei DAI -> vault share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18)
Bob (victim) deposits 100 ether DAI
Bob receives 0 shares
Bob receives 0 shares due to a precision issue. His deposited funds are lost.

The shares are calculated as following
return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
In case of a very high share price, due to totalAssets() > assets * supply, shares will be 0.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a well-known issue, Uniswap and other protocols had similar issues when supply == 0.

For the first deposit, mint a fixed amount of shares, e.g. 10**decimals()

if (supply == 0) {
return 10**decimals;
} else {
return assets.mulDivDown(supply, totalAssets());
}
@code423n4 code423n4 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 Jan 19, 2023
code423n4 added a commit that referenced this issue Jan 19, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #509

@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #588

@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Feb 19, 2023
@c4-judge
Copy link
Contributor

Picodes changed the severity to 3 (High Risk)

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-588 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

2 participants