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

ERC4626 mints token amount, not number of shares #20

Closed
code423n4 opened this issue Feb 18, 2022 · 3 comments
Closed

ERC4626 mints token amount, not number of shares #20

code423n4 opened this issue Feb 18, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/Rari-Capital/solmate/blob/main/src/mixins/ERC4626.sol#L67

Vulnerability details

Impact

If the number of assets is different from the number of shares, the user will get more or less shares than they expect.

Users don't have to be sophisticated at all, just using the contract as intended can cause users to get more or less of the shares of a vault.

Proof of Concept

Here's a proof of concept:

  1. Alice deposits 1e18 tokens into ERC4626 vault.
  2. Bob deposits 1e18 tokens into ERC4626 vault.
  3. The amount of tokens in the vault doubles (maybe due to yield), so there are 4e18 tokens in the vault
  4. Alice calls mint with 1e18 shares.

We would expect the following:
a. Alice now has 2/3rds of the shares (2e18)
b. Alice has to transfer 2e18 tokens

Alice correctly has to transfer 2e18 tokens. But she receives 2e18 shares instead of 1e18 shares because of the line of code here.
https://github.com/Rari-Capital/solmate/blob/main/src/mixins/ERC4626.sol#L67

Recommended Mitigation Steps

Change amount to shares on that line. Also should check other implementations to ensure this isn't exploitable in any production contracts.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 18, 2022
code423n4 added a commit that referenced this issue Feb 18, 2022
@Joeysantoro
Copy link
Collaborator

#18

@Joeysantoro Joeysantoro added the duplicate This issue or pull request already exists label Feb 26, 2022
@GalloDaSballo
Copy link
Collaborator

The warden has identified what is most likely a small oversight, which would have drastic consequences in the internal accounting of the Vault.
Because of impact, I agree with high severity.

Will make this finding primary because it shows some details.

The sponsor has mitigated

@GalloDaSballo
Copy link
Collaborator

Duplicate of #27

@GalloDaSballo GalloDaSballo marked this as a duplicate of #27 Mar 12, 2022
@GalloDaSballo GalloDaSballo added the duplicate This issue or pull request already exists label Mar 12, 2022
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 This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants