Ethos Reserve contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

ReaperVaultV2._withdraw() burns 100% of shares even if the vault balance is less than the required underlying amount.

As a result, users would lose some shares during withdrawal.

Proof of Concept

Users can receive underlying tokens by burning their shares using _withdraw().

If the vault doesn't have enough underlying balance, it withdraws from strategies inside withdrawalQueue.

File: ReaperVaultV2.sol
359:     function _withdraw(
360:         uint256 _shares,
361:         address _receiver,
362:         address _owner
363:     ) internal nonReentrant returns (uint256 value) {
364:         require(_shares != 0, "Invalid amount");
365:         value = (_freeFunds() * _shares) / totalSupply();
366:         _burn(_owner, _shares);
367: 
368:         if (value > token.balanceOf(address(this))) { 
398:             ....
399:             vaultBalance = token.balanceOf(address(this));
400:             if (value > vaultBalance) {
401:                 value = vaultBalance; //@audit should reduce shares accordingly
402:             }
403: 
404:             require(
405:                 totalLoss <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR,
406:                 "Withdraw loss exceeds slippage"
407:             );
408:         }
409: 
410:         token.safeTransfer(_receiver, value);
411:         emit Withdraw(msg.sender, _receiver, _owner, value, _shares);
412:     }

After withdrawing from the strategies of withdrawalQueue, it applies the max cap at L401.

But as we can see from setWithdrawalQueue(), withdrawalQueue wouldn't contain all of the active strategies and the above condition at L400 will be true.

In this case, users will get fewer underlying amounts after burning the whole shares that they requested.

As a reference, it recalculates the shares for the above case in Yearn vault.

    if value > vault_balance:
        value = vault_balance
        # NOTE: Burn # of shares that corresponds to what Vault has on-hand,
        #       including the losses that were incurred above during withdrawals
        shares = self._sharesForAmount(value + totalLoss)

Tools Used

Manual Review

Recommended Mitigation Steps

We should recalculate the shares and burn them rather than burn all shares.

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L109 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L200

Vulnerability details

Impact

ReaperBaseStrategyv4.harvest() might revert in an emergency if there is no position on the lending pool.

As a result, the funds might be locked inside the strategy.

Proof of Concept

The main problem is that Aave lending pool doesn't allow 0 withdrawals.

  function validateWithdraw(
    address reserveAddress,
    uint256 amount,
    uint256 userBalance,
    mapping(address => DataTypes.ReserveData) storage reservesData,
    DataTypes.UserConfigurationMap storage userConfig,
    mapping(uint256 => address) storage reserves,
    uint256 reservesCount,
    address oracle
  ) external view {
    require(amount != 0, Errors.VL_INVALID_AMOUNT);

So the below scenario would be possible.

1. After depositing and withdrawing from the Aave lending pool, the current position is 0 and the strategy is in debt.
2. It's possible that the strategy has some want balance in the contract but no position on the lending pool. It's because _adjustPosition() remains the debt during reinvesting and also, there is an authorizedWithdrawUnderlying() for STRATEGIST to withdraw from the lending pool.
3. If the strategy is in an emergency, harvest() tries to liquidate all positions(=0 actually) and it will revert because of 0 withdrawal from Aave.
4. Also, withdraw() will revert at L98 as the strategy is in the debt.

As a result, the funds might be locked inside the strategy unless the emergency mode is canceled.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check 0 withdrawal in _withdrawUnderlying().

    function _withdrawUnderlying(uint256 _withdrawAmount) internal {
        uint256 withdrawable = balanceOfPool();
        _withdrawAmount = MathUpgradeable.min(_withdrawAmount, withdrawable);

        if(_withdrawAmount != 0) {
            ILendingPool(ADDRESSES_PROVIDER.getLendingPool()).withdraw(address(want), _withdrawAmount, address(this));
        }
    }

Lines of code

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

Vulnerability details

Impact

The locked profit degradation for the past will be changed with the new lockedProfitDegradation.

As a result, malicious users can steal others' rewards by frontrunning.

Proof of Concept

setLockedProfitDegradation() is used to change lockedProfitDegradation by admin.

    function setLockedProfitDegradation(uint256 degradation) external {
        _atLeastRole(ADMIN);
        require(degradation <= DEGRADATION_COEFFICIENT, "Degradation cannot be more than 100%");
        lockedProfitDegradation = degradation;
        emit LockedProfitDegradationUpdated(degradation);
    }

And lockedProfitDegradation is used to calculate the locked profit.

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

        return 0;
    }

But it doesn't update lockedProfit and lastReport before changing lockedProfitDegradation so the below scenario would be possible.

1. Let's assume lockedProfit = 200, lastReport = block.timestamp after calling report(), lockedProfitDegradation are 6 hours in blocks.
2. 3 hours later, 100 tokens of lockedProfit are released and added to the free funds. We can assume report() wasn't called for 3 hours.
3. At that time, lockedProfitDegradation is changed to 4 hours in blocks and it means 200 * 3 / 4 = 150 tokens are released. As a result, free funds are increased by 50 inside the same block.
4. So a malicious user(it should be a pool) can front run deposit() with huge amounts before lockedProfitDegradation is changed and charge most of the new rewards(=50).

Similarly, already unlocked funds will be treated as locked again if lockedProfitDegradation is decreased.

Even if there is no front run as all depositors are pools, it's not fair to change locked/unlocked amounts that are confirmed already.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify setLockedProfitDegradation() like below.

    function setLockedProfitDegradation(uint256 degradation) external {
        _atLeastRole(ADMIN);
        require(degradation <= DEGRADATION_COEFFICIENT, "Degradation cannot be more than 100%");

        // update lockedProfit and lastReport
        lockedProfit = _calculateLockedProfit();
        lastReport = block.timestamp;

        lockedProfitDegradation = degradation;
        emit LockedProfitDegradationUpdated(degradation);
    }

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L251 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L282 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L288

Vulnerability details

Impact

The _rebalance() reverts if a strategy gets loss. Because _rebalance() is called on all important workflows, this leads to insolvency of the protocol.

Proof of Concept

The protocol uses ReaperVaultERC4626 to manage the collateral assets and farm profit. The vaults are connected to whitelisted strategies.

But it is not guaranteed that the strategies earn profit all the time.

On the other hand, in several places of ActivePool._rebalance(), the protocol assumes that it can get more than deposits all the time.

At L251, where the protocol calculates the profit by subtracting the stored yieldingAmount from the sharesToAssets, this will revert if the strategy got loss.

ActivePool.sol
242:         // how much has been allocated as per our internal records?
243:         vars.currentAllocated = yieldingAmount[_collateral];//@audit-info current yield deposit
244:
245:         // what is the present value of our shares?
246:         vars.yieldGenerator = IERC4626(yieldGenerator[_collateral]);
247:         vars.ownedShares = vars.yieldGenerator.balanceOf(address(this));
248:         vars.sharesToAssets = vars.yieldGenerator.convertToAssets(vars.ownedShares);
249:
250:         // if we have profit that's more than the threshold, record it for withdrawal and redistribution
251:         vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);//@audit-issue profit from the farming, this can revert!
252:         if (vars.profit < yieldClaimThreshold[_collateral]) {
253:             vars.profit = 0;
254:         }

At L282 where the protocol withdraws specifying the collateral amount to receive but it will revert if the strategy lost.

ActivePool.sol
276:         // + means deposit, - means withdraw
277:         vars.netAssetMovement = int256(vars.toDeposit) - int256(vars.toWithdraw) - int256(vars.profit);
278:         if (vars.netAssetMovement > 0) {
279:             IERC20(_collateral).safeIncreaseAllowance(yieldGenerator[_collateral], uint256(vars.netAssetMovement));
280:             IERC4626(yieldGenerator[_collateral]).deposit(uint256(vars.netAssetMovement), address(this));
281:         } else if (vars.netAssetMovement < 0) {
282:             IERC4626(yieldGenerator[_collateral]).withdraw(uint256(-vars.netAssetMovement), address(this), address(this));//@audit-info coll received, this can revert
283:         }

Because _rebalance() is called on all important workflows, this leads to insolvency of the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not assume sharesToAssets>yieldingAmount at all places mentioned and handle appropriately.

Low severity findings

[L-01] Core contracts are susceptible to initializer re-entrancy due to OpenZeppelin version advisory

The version of OpenZeppelin in the core package falls within this advisory, so advised to upgrade to avoid accidentally adding a vulnerability if external calls are made.

[L-02] Re-entrancy risk in openTrove

• https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L526 Although unlikely bad collateral will be included, it is recommended to keep CEI pattern. Otherwise, malicious strategy can reenter report() function and cause problems. (It is understood strategy is added by admin and this is very unlikely)

[L-03] No rewards swapped in _harvestCore

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

When steps are not initialized it seems admin has to setHarvestSteps first, but this may not happen so rewards aren't swapped.

[L-04] Call _disableInitializers()

• https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L61 It is recommended to call _disableInitializers() in the initializer function.

[L-05] Wrong event string

• https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L155 require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS"); I believe the code is correct and the event string is misleading here, should be changed into 2000 BPS or 20%.

[L-06] Wrong comments

• https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L660 "collateral ratio" → "nominal .."
• https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L49 "ETH" → "collateral"

[L-07] Double entrypoint token can break vault if inCaseTokensGetStuck called with legacy address

ReaperVaultV2::inCaseTokensGetStuck can be called by an admin to withdraw tokens which are mistakenly sent to the vault. A double entrypoint token messes this up to cause complete withdrawal, breaking the vault.

Although it is understood collateral tokens are added by admin; it certainly wouldn't be ideal as collaterals are immutable and USDT being upgradeable/high quality/large market cap collateral makes it a good candidate for this vulnerability.

[L-08] Inconsistency between previewDeposit and deposit of ReapervaultV2

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultERC4626.sol#L52 https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L334 If _freeFunds()==0, the ReapervaultV2::previewDeposit returns the same amount to the input assets value. But in actual deposit, it reverts when freeFunds==0 at ReaperVaultV2.sol#L334

[L-09] Sanity check in ReaperVaultV2::setLockedProfitDegradation

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L617 lockedProfitDegradation should be greater than zero or else funds are not released. It is understood that it is reversible by calling the same function with a correct value, but recommend to add this check.

Refactoring findings

[R-01] Use emit keyword for events

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/ActivePool.sol#L194; https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/ActivePool.sol#L201; https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/SortedTroves.sol#L198

[R-02] Inconsistent use of uint/uint256

In many places, the protocol is using uint and uint256. It is recommended to use consistent type names.

[R-03] Misleading variable name

• https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/TroveManager.sol#L715 Based on the context it is recommended to rename _troveArray into _troveOwnerArray.

Noncritical findings

[N-01] Use more explicit modifier names

• https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/CollateralConfig.sol#L128

The modifier checkCollateral can be renamed to be more explicit with what it is "checking", for example:

isValidCollateral
isAllowedCollateral
requireValidCollateral

[N-02] withdrawFromSp doesn't _requireNonZeroAmount() unlike provideToSP

Although this does not seems to cause problems, it is recommended to keep consistency for these functions.

[N-03] Add access control to the ReaperStrategygranarySupplyOnly.initialize()

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

[N-04] Remove console.log imports

All instances should be removed.

[N-05] Missing info in the emitted event string

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/ActivePool.sol#L327-L335 It is good to include the actual redemptionHelper address value.