Ethos Reserve contest - 0xTheC0der's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L156-L160 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L205-L217 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L191-L199 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L227-L231 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/test/starter-test.js#L1066-L1082

Vulnerability details

Impact

An emergency exit of a strategy can only be triggered with guardian or higher privileges. As a result all funds of the strategy are deposited into the vault on the next harvest.
However, strategist privileges (lower than guardian) are sufficient to reduce the deposit amout such that up to 1/9 of the strategy funds are still left in the strategy contract after harvest.

Bug description

Strategy.setEmergencyExit() (called by guardian) calls Vault.revokeStrategy() which sets the allocBPS of the strategy to zero. This is necessary to make Vault.availableCapital() return a value <= 0 for the strategy such that all strategy funds are deposited into the vault when calling Strategy.harvest().
However, Vault.updateStrategyAllocBPS() (called by strategist) allows to restore a non-zero value of allocBPS. As a result, stratMaxAllocation can become > stratCurrentAllocation in Vault.availableCapital() which therefore returns a value > 0. Consequently, a following call to Strategy.harvest() will not deposit all funds into the vault. Depending on allocBPS and totalAllocBPS (depends on number of strategies) up to 1/9 of funds are left in the strategy contract after emergency exit.

It should not be possible in any case that the emergency exit amout is reduced using lower privileges (strategist) than initially required (guardian) to trigger the emergency exit.
Assets are not at direct risk since they are still within the protocol, but the function of the protocol or its availability is impacted because the vault contract does not receive the expected funds in case of emergency.
According to Severity Categorization: 2 — Med

Proof of Concept

Side quest: Fix test cases "Emergency scenarios" by replacing L1063 and L1081 with expect(stratBalance).to.be.eq(toWantUnit('0')); such that an empty strategy balance is required after emergency shutdown/exit. However, this is just for "beauty" since in case of an incomplete emergency harvest, the test cases already fail a few lines above when checking the vault balance, see "Main quest".

Main quest:

1. Fix test case "Strategy should handle emergency exit" by replacing L1076 with await strategy.setEmergencyExit();. Because at the moment this test case is completely the same as "Vault should handle emergency shutdown".

2. Provoke the bug by adding await vault.connect(strategist).updateStrategyAllocBPS(strategy.address, 10000); after L1076.

3. Run this test case and see that the vault does not receive the expected funds: AssertionError: expected 900000026 to be at least 1000000000.

4. Optional: Print the remaining balance of the strategy contract to see that it's > 0, i.e. emergency exit is incomplete.

Test diff:

diff --git a/starter-test.js.orig b/starter-test.js
index 4069176..5a8539f 100644
--- a/starter-test.js.orig
+++ b/starter-test.js
@@ -1060,10 +1060,10 @@ describe('Vaults', function () {
       vaultBalance = await want.balanceOf(vault.address);
       expect(vaultBalance).to.be.gte(toWantUnit('10'));
       stratBalance = await strategy.balanceOf();
-      expect(stratBalance).to.be.gte(toWantUnit('0'));
+      expect(stratBalance).to.be.eq(toWantUnit('0'));
     });

-    it('Strategy should handle emergency exit', async function () {
+    it('Strategy should handle emergency exit (strategist should not be able to interfere)', async function () {
       const {vault, strategy, want, wantHolder} = await loadFixture(deployVaultAndStrategyAndGetSigners);
       await vault.connect(wantHolder)['deposit(uint256)'](toWantUnit('10'));
       await strategy.harvest();
@@ -1073,12 +1073,13 @@ describe('Vaults', function () {
       let stratBalance = await strategy.balanceOf();
       expect(stratBalance).to.be.gte(toWantUnit('9'));

-      await vault.setEmergencyShutdown(true);
+      await strategy.setEmergencyExit();
+      await vault.connect(strategist).updateStrategyAllocBPS(strategy.address, 10000);

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

Flag a strategy as revoked in Vault.revokeStrategy() and therefore do not allow subsequent changes of allocBPS in Vault.updateStrategyAllocBPS() by checking for this flag. A guardian (or higher) should have the privilege to clear the revoked flag, or clear it automatically during emergency harvest.

Low severtity findings

Vault roles - centralization risk

Require that the addresses in _strategists[] and _multisigRoles[] in ReaperBaseStrategyv4 .__ReaperBaseStrategy_init as well as ReaperVaultV2.constructor are all different from each other.

Wrong NatSpec comment about decimals

According to the NatSpect comment of ReaperVaultV2.getPricePerFullShare, the functions returns an uint256 with 18 decimals. However, the code shows that the underlying token's decimals are mirrored which might be != 18. This function is not called by any other contract under scope, but an external contract might be at risk when relying on the wrong comment.

Limited upgradeability of ReaperStrategyGranarySupplyOnly contract

The upgradeable contract ReaperStrategyGranarySupplyOnly inherits from the non-stateless contract ReaperBaseStrategyv4 which does not contain a storage gap (e.g. uint256[50] private __gap;) after its storage variables. Therfore, adding another storage variable to ReaperBaseStrategyv4 will shift down the storage in the inheritance chain. This will break the ReaperStrategyGranarySupplyOnly contract in case of an upgrade. Consider adding a gap to ReaperBaseStrategyv4 in order to reserve space for future storage variables.

Non-critical findings

Vault depositor role not granted to anyone

The vault DEPOSITOR role is not granted to anyone within the whole codebase. I suppose the role is granted on demand by the default admin after deployment. However, you might also want to handle this within ReaperVaultV2.constructor like you do with the other roles.

ToDo's in StabilityPool contract

There are two ToDo's (#1 and #2) about unused local variables in the StabilityPool contract.