First depositor attack: first depositor can steal funds from later depositors by manipulating initial share price.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L110-L112
https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L202-L210
https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L51-L54
https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L66-L69

Vulnerability details

Impact

Detailed description of the impact of this finding.
First depositor can steal funds from later depositors by manipulating initial share price.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how the first depositor, Bob, can steal funds from later depositors by manipulating initial share price below:

1. Initially _freeFunds() = 0 and totalSupply(), note the implementation of _freeFunds() below. In this attack, totalAllocated and the locked profit will be zero initially and will not change in this attack. We will only pay attention to the token.balanceOf(address(this)) component for the _freeFunds().

 function _freeFunds() internal view returns (uint256) {
        return balance() - _calculateLockedProfit();
    }

function balance() public view returns (uint256) {
        return token.balanceOf(address(this)) + totalAllocated;
}

 function _calculateLockedProfit() internal view returns (uint256) {
        uint256 lockedFundsRatio = (block.timestamp - lastReport) * lockedProfitDegradation;
        if (lockedFundsRatio < DEGRADATION_COEFFICIENT) {
            return lockedProfit - ((lockedFundsRatio * lockedProfit) / DEGRADATION_COEFFICIENT);
        }

        return 0;
    }

2. Bob calls deposit(1, Bob) to spend 1 wei of asset token in exchange of 1 vault share. After that, we have totalSupply()=1, and _freeFunds() = 1.

 function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        if (totalSupply() == 0 || _freeFunds() == 0) return assets;
        return (assets * totalSupply()) / _freeFunds();
    }

3. Bob sends (1,000*1e18) asset tokens to the ReaperVaultERC4626 contract. After that, we have totalSupply()=1, and _freeFunds() = 1,000*1e18+1. Now the vault share price is inflated to (1,000*1e18+1)/share.

4. Alice calls Deposit(2,000*1e18, Alice) and gets 1 vault share. After that, we have totalSupply()=2, and _freeFunds() = 3,000*1e18+1. Now the vault share price is (1,500*1e18)/share.

5. Bob withdraws his one share by calling redeem(1, Bob, Bob) and he gets back 1,500*1e18 asset tokens. We do not need to worry about the rest of the code after if (value > token.balanceOf(address(this))) since the condition is false.

 function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) external override returns (uint256 assets) {
        if (msg.sender != owner) _spendAllowance(owner, msg.sender, shares);
        assets = _withdraw(shares, receiver, owner);
    }

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"
            );
        }

6. If Alice withdrew her vault share at this point, she would get 1,5001e18 asset tokens and lose 5001e18 asset tokens.

7. Bob gains 500*1e18 asset tokens, He steals it from Alice!

Tools Used

VScode

Recommended Mitigation Steps

• When the vault is deployed in a factory, an honest deployer can mint 1000 shares with 1000 wei and sent them to a dead address.

• Enforce a minimum 1000 shares for each deposit and automatically sends 1000 shares to a dead address from the first deposit. The amount of 1000 wei is negligible for most asset tokens.

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