Ethos Reserve contest - Koolex's results

Lines of code

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

Vulnerability details

Impact

When withdrawing from a vault, the user passes the amount of shares to be burnt, and in return the user receives the corresponding assets as follows:

value = (_freeFunds() * _shares) / totalSupply();

If there is not enough balance in the vault, then it withdraws from strategies in the order they were added in withdrawalQueue. However, in case the strategies don't have enough balance, the vault burns all user's shares even though s/he hasn't received all of the corresponding assets for it.

In other words, if the user should receive Y tokens when passing X shares and the issue occurs, then X shares will be burnt even though the user receives less than Y which is not fair.

Proof of Concept

Let's break down _withdraw's flow:

1. Calculate how many tokens the user should receive

   value = (_freeFunds() * _shares) / totalSupply();

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

2. Burn the shares

   _burn(_owner, _shares);

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

3. Assume there’s not enough fund in the vault then it will loop through withdrawalQueue and withdraw what's required to cover the gap. https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L372-L397

4. Assume the tokens the user should receive is still lower than the balance then update it to the balance

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

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

5. Finally transfer the tokens to the user

 token.safeTransfer(_receiver, value);

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

As you can notice, no matter how many tokens the user receives, s/he will always get his/her shares burnt.

Tools Used

Manual analysis

Recommended Mitigation Steps

1. Move the burning logic to the end.
2. In case, there's still not enough fund then recalculate how many shares should be burnt based on the new value of value

For example:

	vaultBalance = token.balanceOf(address(this));
	if (value > vaultBalance) {
		value = vaultBalance;
		=> recalculate the shares to be burnt
	}
	
  _burn(_owner, _shares);
	token.safeTransfer(_receiver, value);

Lines of code

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

Vulnerability details

Impact

When withdrawing from a vault, the user passes the amount of shares to be burnt, and in return the user receives the underlying token. If there is not enough balance in the vault, then it withdraws from strategies in the order they were added in withdrawalQueue till it has enough amount for withdrawal.

However, if a withdraw call to one strategy fails (i.e. reverts) for whatever reason, then the withdrawal will revert even if other strategies could successfully cover the withdrawal amount. Thus, one strategy could block other strategies from being utilised resulting in blocking withdrawal functionality in the protocol.

As the protocol supports multiple strategies, and those strategies basically rely on external DeFis (e.g. lending markets such as Granary), the protocol's withdrawal is vulnerable to being blocked since the hard/heavy dependency. One scenario in Granary that could possibly cause this, is when the lending pool is paused. Here is the sequence flow for clearer picture:

Vault = ReaperVaultV2 Strategy = ReaperStrategyGranarySupplyOnly LendingPool = Granary's Lending Pool

flowchart TD;
   	subgraph Vault
   Vault.withdraw-->Vault._withdraw(_withdraw)
   end
   	subgraph Strategy
Strategy.withdraw-->Strategy._liquidatePosition(_liquidatePosition);
   Strategy._liquidatePosition-->Strategy._withdraw(_withdraw);
   	Strategy._withdraw-->Strategy._withdrawUnderlying(_withdrawUnderlying);
   	end
   	subgraph LendingPool
LendingPool.withdraw;		
   	end
   	Vault-->Strategy
   	Strategy-->LendingPool

Setting this to high severity due to the following:

1. The impact goes beyond the strategy itself.
2. Withdrawal function is blocked till this blocking strategy works again. Please note that the only way to remove the blocking strategy is by setting the withdrawal queue again by the admin via setWithdrawalQueue. Which is not a good solution, as the strategy could resume working again (i.e. it was failing temporarily)

Proof of Concept

• ReaperVaultV2.withdraw

uint256 loss = IStrategy(stratAddr).withdraw(Math.min(remaining, strategyBal));

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

• ReaperStrategyGranarySupplyOnly.withdraw

(amountFreed, loss) = _liquidatePosition(_amount);

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L101

• ReaperStrategyGranarySupplyOnly._liquidatePosition

if (wantBal < _amountNeeded) {
            _withdraw(_amountNeeded - wantBal);
            liquidatedAmount = balanceOfWant();
        } else {
            liquidatedAmount = _amountNeeded;
        }

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L93

• ReaperStrategyGranarySupplyOnly._withdraw => _withdrawUnderlying

ILendingPool(ADDRESSES_PROVIDER.getLendingPool()).withdraw(address(want), _withdrawAmount, address(this));

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L189

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L200

• Granary's Lending Pool Provider
  • Contract: 0xdDE5dC81e40799750B92079723Da2acAF9e1C6D6
  • getLendingPool function which returns the proxy that returns the lending pool address https://optimistic.etherscan.io/address/0xdde5dc81e40799750b92079723da2acaf9e1c6d6#readContract#F3
• Granary's Lending Pool
  • Contract address returned by the proxy returned by the provider: 0x72fAD09e3DA8Cec0e975bF253c1e5EaFdb927FEc
  • Withdraw function https://optimistic.etherscan.io/address/0x72fad09e3da8cec0e975bf253c1e5eafdb927fec#code#F1#L142

As you will notice in the withdraw function, there is a modifier whenNotPaused. If this function is paused, it reverts, and all sequence calls will do as well. Check the sequence diagram above.

Tools Used

Manual analysis

Recommended Mitigation Steps

One possibility is using try/catch, when the strategy withdrawal reverts, keep going to next strategy for withdrawal. This way, the strategy withdrawal impact is isolated and withdrawal from other strategies can still work.