Initial vault deposits can be frontrun allowing attacker to steal entire deposit

[code423n4]

Lines of code

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

Vulnerability details

Impact

Attacker can steal initial deposits into the ReaperVaultV2 contract by frontrunning.

Proof of Concept

The first deposit (or any deposit where the totalSupply is 0) in a ReaperVaultV2 contract mints shares equivalent to the amount of tokens being deposited. Every other deposit will mint according to: (_amount * totalSupply()) / freeFunds.

if (totalSupply() == 0) {
	shares = _amount;
} else {
	shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"
}

An attacker can take advantage of this logic as follows:

1. Victim goes to make first deposit of 100e18 tokens
2. Attacker frontruns victim by depositing 1 wei, receiving 1 share. In the same transaction, the attacker sends 101e18 tokens to the contract such that the token balance of the contract is 101e18. Since none of the tokens have been deposited in a yield strategy, freeFunds is also 101e18.
3. Victims transaction proceeds and the victim receives 0 shares because 100e18 * 1 / 101e18 = 0.9900990099009901 which is rounded down to 0.
4. Attacker now has 100% of the vault shares, corresponding to all 201e18 tokens and withdraws all of the tokens.

Although this logic is simplified to interacting with the deposit method directly, which is gated via _atLeastRole(DEPOSITOR), this attack is still possible via opening troves.

Opening a trove results in sending collateral to ActivePool:

// openTrove() - BorrowerOperations.sol:L232
_activePoolAddColl(contractsCache.activePool, _collateral, _collAmount);

// BorrowerOperations.sol:L505-508
function _activePoolAddColl(IActivePool _activePool, address _collateral, uint _amount) internal {
	IERC20(_collateral).safeIncreaseAllowance(address(_activePool), _amount);
	_activePool.pullCollateralFromBorrowerOperationsOrDefaultPool(_collateral, _amount);
}

Which triggers a rebalance:

// pullCollateralFromBorrowerOperationsOrDefaultPool() - ActivePool.sol:L211
_rebalance(_collateral, 0);

Which then makes a deposit into the vault:

// _rebalance() - ActivePool.sol:L280
IERC4626(yieldGenerator[_collateral]).deposit(uint256(vars.netAssetMovement), address(this));

As we can see, we can perform the above example in an indirect manner by observing troves to be opened in the mempool and frontrunning them by opening our own troves with 1 wei of collateral and sending a larger amount of collateral than the victims deposit directly to the vault contract.

It is further possible for the attacker to continue to steal additional deposits so long as they have the capital to maintain a vault contract balance greater than the current victim deposit, stealing every deposit until an emergencyShutdown is enacted.

Recommended Mitigation Steps

There are two possible mitigation strategies which can be taken:

1. Take a similar approach to that of Uniswap V2 and send an initial bunch of tokens, e.g. 1000 tokens, to the zero address. Uniswap V2 implementation
2. Revert if the amount of shares to be minted is 0, e.g. require(shares != 0, "No vault tokens minted").

[c4-judge]

trust1995 marked the issue as duplicate of #848

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)