ReaperVaultV2 inflation attack

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L334

Vulnerability details

Impact

A hacker can drain new deposits into the pool by causing a rounding error in a share-issuing function:

shares = (_amount * totalSupply()) / freeFunds;

The ReaperVaultV2._deposit() method issues zero shares for the victim if the freeFunds (which is proportional to token.balanceOf(address(vault))) is greater than _amount * totalSupply() (which may be the case if the totalSupply() is low).

The impact is not high because a hacker can only deposit into the pool via contracts that have the DEPOSITOR role (e.g. ActivePool).

Proof of Concept

Let's consider a scenario where there are two contracts that have the DEPOSITOR role. We will refer to the first contract as the HACKER and the second as the VICTIM.

1. The HACKER creates a small number of shares. For example, they may issue a single share by calling vault.deposit(1 wei).
2. The VICTIM intends to deposit a significant amount of funds into the vault, such as 20_000e18 wei.
3. The HACKER anticipates the VICTIM's transaction and front-runs it with a direct donation to the pool by calling token.transfer(vault, 20_000e18).
4. The VICTIM's transaction is processed. However, the VICTIM does not receive any shares:

shares == (_amount * totalSupply()) / freeFunds 
       == (20_000e18 * 1) / (20_000e18 + 1)
       == 0

This happens because the freeFunds are proportional to the vault's balance and hacker can manipulate it by direct donations:

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

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L288

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

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L279

Tools Used

Manual

Recommended Mitigation Steps

To prevent errors, it is recommended to add a requirement that shares cannot be equal to zero.

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[tess3rac7]

We have received this report over and over through the last year or so across different platforms. DEPOSITOR role is trusted and will only be granted to ActivePool.

In the more general case (outside of Ethos), Reaper's vaults always undergo manual testing whereby funds are deposited before we invite the public to utilize the vault.

I will not dispute the validity since this is a valid scenario, but it will never occur in the case of Ethos (since only one DEPOSITOR being ActivePool), and never really occur otherwise outside of ethos. Recommend low severity.

[c4-sponsor]

tess3rac7 marked the issue as disagree with severity

[trust1995]

I agree with sponsor that the finding severity is low, given the centralized nature of the DEPOSITOR role.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-b