Yeti Finance contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

Waste of gas

Proof of Concept

There's a redondant check on ThreePieceWiseLinearPriceCurve.sol: percentBacked >= 0, but percentBacked is of type uint, therefore it's always true.

https://github.com/code-423n4/2021-12-yetifinance/blob/1da782328ce4067f9654c3594a34014b0329130a/packages/contracts/contracts/PriceCurves/ThreePieceWiseLinearPriceCurve.sol#L131

Tools Used

VS Code

Recommended Mitigation Steps

Delete the >= 0 check

Handle

Dravee

Vulnerability details

Impact

++i costs less gass compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Proof of Concept

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

./ActivePool.sol:133:        for (uint i = 0; i < poolColl.tokens.length; i++) {
./ActivePool.sol:167:        for (uint i = 0; i < _tokens.length; i++) {
./ActivePool.sol:184:        for (uint i = 0; i < _tokens.length; i++) {
./BorrowerOperations.sol:699:        for (uint256 i = 0; i < _tokensIn.length; i++) {
./BorrowerOperations.sol:737:        for (uint256 i = 0; i < len; i++) {
./BorrowerOperations.sol:873:        for (uint256 i = 0; i < _colls.length; i++) {
./BorrowerOperations.sol:890:        for (uint256 i = 0; i < arr.length; i++) {
./BorrowerOperations.sol:920:        for (uint256 i = 0; i < _colls1.length; i++) {
./BorrowerOperations.sol:929:        for (uint256 i = 0; i < _colls.length; i++) {
./BorrowerOperations.sol:1061:        for (uint i = 0; i < _routers.length; i++) {
./BorrowerOperations.sol:1068:        for (uint i = 0; i < _indices.length - 1; i++) {
./DefaultPool.sol:97:        for (uint256 i = 0; i < poolColl.tokens.length; i++) {
./DefaultPool.sol:133:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/LiquityBase.sol:63:        for (uint i = 0; i < _coll.tokens.length; i++) {
./Dependencies/LiquityBase.sol:97:        for (uint i = 0; i < _tokens.length; i++) {
./Dependencies/LiquityBase.sol:106:        for (uint i = 0; i < _colls.tokens.length; i++) {
./Dependencies/LiquityBase.sol:115:        for (uint i = 0; i < _colls.tokens.length; i++) {
./Dependencies/LiquityBase.sol:149:        for (uint i = 0; i < tokens.length; i++) {
./Dependencies/LiquityBase.sol:158:        for (uint i = 0; i < coll.tokens.length; i++) {
./Dependencies/LiquityBase.sol:168:        for (uint i = 0; i < _coll.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:36:        for (uint256 i = 0; i < _coll1.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:44:        for (uint256 i = 0; i < _coll2.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:59:        for (uint256 i = 0; i < coll3.tokens.length; i++) {      
./Dependencies/YetiCustomBase.sol:104:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:122:        for (uint256 i = 0; i < _subTokens.length; i++) {       
./Dependencies/YetiCustomBase.sol:144:        for (uint256 i = 0; i < _coll1.tokens.length; i++) {    
./Dependencies/YetiCustomBase.sol:152:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:165:        for (uint256 i = 0; i < coll3.tokens.length; i++) {     
./Dependencies/YetiCustomBase.sol:178:        for (uint i = 0; i < _arr.length; i++) {
./HintHelpers.sol:139:        for (uint256 i = 0; i < colls.tokens.length; i++) {
./HintHelpers.sol:190:            i++;
./MultiTroveGetter.sol:110:        for (uint i = 0; i < data.allColls.length; i++) {
./StabilityPool.sol:562:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:588:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:592:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:628:        for (uint256 i = 0; i < _assets.length; i++) {
./StabilityPool.sol:720:        for (uint256 i = 0; i < assets.length; i++) {
./StabilityPool.sol:942:        for (uint256 i = 0; i < assets.length; i++) {
./StabilityPool.sol:1011:        for (uint256 i = 0; i < allColls.length; i++) {
./TeamAllocation.sol:66:        for (uint i = 0; i < 7; i++) {
./TroveManager.sol:234:        for (uint i = 0; i < _lowerHints.length; i++) {
./TroveManager.sol:348:        for (uint i = 0; i < allColls.length; i++) {
./TroveManager.sol:374:        for (uint i = 0; i < allColls.length; i++ ) {
./TroveManager.sol:397:        for (uint i = 0; i < allColls.length; i++ ) {
./TroveManager.sol:420:        for (uint i = 0; i < assets.length; i++) {
./TroveManager.sol:460:        for (uint i = 0; i < borrowerColls.length; i++) {
./TroveManager.sol:476:        for (uint i = 0; i < Troves[_borrower].colls.tokens.length; i++) {     
./TroveManager.sol:525:        for (uint i = 0; i < _tokens.length; i++) {
./TroveManager.sol:582:        for (uint i = 0; i < allColls.length; i++) {
./TroveManager.sol:603:        for (uint i = 0; i < _tokens.length; i++) {
./TroveManagerLiquidations.sol:255:        for (vars.i = 0; vars.i < _troveArray.length; vars.i++) {
./TroveManagerLiquidations.sol:334:        for (vars.i = 0; vars.i < _troveArray.length; vars.i++) {
./TroveManagerLiquidations.sol:394:        for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:475:        for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:701:            for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:721:            for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:808:        for (uint i = 0; i < _troveTokens.length; i++) {
./TroveManagerLiquidations.sol:840:        for (uint i = 0; i < _colls.tokens.length; i++) {
./TroveManagerRedemptions.sol:304:        for (uint256 i = 0; i < colls.tokens.length; i++) {
./TroveManagerRedemptions.sol:367:            for (uint256 i = 0; i < colls.tokens.length; i++) {     
./TroveManagerRedemptions.sol:517:        for (uint256 i = 0; i < coll.amounts.length; i++) {
./YETI/BoringCrypto/BoringBatchable.sol:37:        for (uint256 i = 0; i < calls.length; i++) {       
./YETI/BoringCrypto/BoringERC20.sol:21:                i++;
./YETI/BoringCrypto/BoringERC20.sol:24:            for (i = 0; i < 32 && data[i] != 0; i++) {

j++ is also used:

./BorrowerOperations.sol:921:            for (uint256 j = 0; j < _colls2.length; j++) {
./BorrowerOperations.sol:930:            for (uint256 j = i + 1; j < _colls.length; j++) {
./Dependencies/YetiCustomBase.sol:63:                j++;
./Dependencies/YetiCustomBase.sol:169:                j++;

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable (do the same for j++)

Handle

Dravee

Vulnerability details

Impact

Not getting refunded with the remaining gas on failure on solc < 0.8.0

Proof of Concept

Most of the contracts use solc 0.6.11. Between solc 0.4.10 and 0.8.0, require() used REVERT (0xfd) opcode which refunded remaining gas on failure while assert() used INVALID (0xfe) opcode which consumed all the supplied gas. (see here). require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking. From the current usage of assert, my guess is that most of them can be replaced with require, unless a Panic really is intended.

Here are the assert locations:

./BorrowerOperations.sol:165:        assert(MIN_NET_DEBT > 0);
./BorrowerOperations.sol:290:        assert(vars.compositeDebt > 0);
./LPRewards/Pool2Unipool.sol:213:        assert(_reward > 0);
./LPRewards/Pool2Unipool.sol:214:        assert(_reward == yetiToken.balanceOf(address(this)));
./LPRewards/Pool2Unipool.sol:215:        assert(periodFinish == 0);
./LPRewards/Pool2Unipool.sol:229:            assert(periodFinish > 0);
./LPRewards/Pool2Unipool.sol:256:        assert(account != address(0));
./LPRewards/Unipool.sol:198:        assert(_reward > 0);
./LPRewards/Unipool.sol:199:        assert(_reward == yetiToken.balanceOf(address(this)));
./LPRewards/Unipool.sol:200:        assert(periodFinish == 0);
./LPRewards/Unipool.sol:214:            assert(periodFinish > 0);
./LPRewards/Unipool.sol:241:        assert(account != address(0));
./StabilityPool.sol:569:        assert(_debtToOffset <= _totalYUSDDeposits);
./StabilityPool.sol:611:        assert(_YUSDLossPerUnitStaked <= DECIMAL_PRECISION);
./StabilityPool.sol:656:        assert(newP > 0);
./TroveManager.sol:502:            assert(totalStakesSnapshot[token] > 0);
./TroveManager.sol:571:        assert(closedStatus != Status.nonExistent && closedStatus != Status.active);
./TroveManager.sol:640:        assert(troveStatus != Status.nonExistent && troveStatus != Status.active);
./TroveManager.sol:646:        assert(index <= idxLast);
./TroveManager.sol:672:        assert(newBaseRate > 0);
./TroveManager.sol:743:        assert(decayedBaseRate <= DECIMAL_PRECISION);  // The baseRate can decay to 0
./TroveManagerLiquidations.sol:578:            assert(_YUSDInStabPool != 0);
./TroveManagerRedemptions.sol:164:        assert(contractsCache.yusdToken.balanceOf(_redeemer) <= totals.totalYUSDSupplyAtStart);
./YETI/CommunityIssuance.sol:84:        assert(YETIBalance >= YETISupplyCap);
./YETI/CommunityIssuance.sol:117:        assert(cumulativeIssuanceFraction <= DECIMAL_PRECISION); // must be in range [0,1]
./YUSDToken.sol:229:        assert(sender != address(0));
./YUSDToken.sol:230:        assert(recipient != address(0));
./YUSDToken.sol:238:        assert(account != address(0));
./YUSDToken.sol:246:        assert(account != address(0));
./YUSDToken.sol:254:        assert(owner != address(0));
./YUSDToken.sol:255:        assert(spender != address(0));

Tools Used

VS Code

Recommended Mitigation Steps

Upgrade to solc 0.8.0+ or replace assert by require

Handle

Dravee

Vulnerability details

Impact

In some cases, having function arguments in calldata instead of memory is more optimal. This is a similar finding as https://github.com/code-423n4/2021-09-sushitrident-findings/issues/119

Consider the following generic example:

contract C {
    function add(uint[] memory arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above example, the dynamic array arr has the storage location
memory. When the function gets called externally, the array values are
kept in calldata and copied to memory during ABI decoding (using the
opcode calldataload and mstore). And during the for loop, arr[i]
accesses the value in memory using a mload. However, for the above
example this is inefficient. Consider the following snippet instead:

contract C {
    function add(uint[] calldata arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above snippet, instead of going via memory, the value is directly
read from calldata using calldataload. That is, there are no
intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with
copying value from calldata to memory in a for loop. Each iteration
would cost at least 60 gas. In the latter example, this can be
completely avoided. This will also reduce the number of instructions and
therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument
is only read.

Note that in older Solidity versions, changing some function arguments
from memory to calldata may cause "unimplemented feature error".
This can be avoided by using a newer (0.8.*) Solidity compiler.

Proof of Concept

ActivePool.sol:
  164:     function sendCollaterals(address _to, address[] memory _tokens, uint[] memory _amounts) external override returns (bool) {
  181:     function sendCollateralsUnwrap(address _to, address[] memory _tokens, uint[] memory _amounts, bool _collectRewards) external override returns (bool) {

TroveManager.sol:
  230:     function updateTroves(address[] memory _borrowers, address[] memory _lowerHints, address[] memory _upperHints) external {
  508:     function redistributeDebtAndColl(IActivePool _activePool, IDefaultPool _defaultPool, uint _debt, address[] memory _tokens, uint[] memory _amounts) external override {
  601:     function updateSystemSnapshots_excludeCollRemainder(IActivePool _activePool, address[] memory _tokens, uint[] memory _amounts) external override {

Tools Used

VS Code

Recommended Mitigation Steps

Change memory to calldata for external function parameters

Handle

Dravee

Vulnerability details

Impact Increased gas costs on BoringERC20.sol

Proof of Concept On L19, we use a uint8 as the type for i, the for loop variable: https://github.com/code-423n4/2021-12-yetifinance/blob/1da782328ce4067f9654c3594a34014b0329130a/packages/contracts/contracts/YETI/BoringCrypto/BoringERC20.sol#L19

Due to how the EVM natively works on 256 bit numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

It is only beneficial to use reduced-size arguments if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation. When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values.

Recommended Mitigation Steps Change i to be a uint256.

Handle

Dravee

Vulnerability details

Impact

Using newer compiler versions and the optimizer gives gas optimizations
and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than
  library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an
  additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means
  additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Proof of Concept

The contest repository contracts mainly contain pragma 0.6.11, with some 0.6.12 and a >=0.5.0. The contracts pragma version should be updated to 0.8.4.

Instances include:

./ActivePool.sol:3:pragma solidity 0.6.11;
./BorrowerOperations.sol:3:pragma solidity 0.6.11;
./CollSurplusPool.sol:3:pragma solidity 0.6.11;
./DefaultPool.sol:3:pragma solidity 0.6.11;
./Dependencies/AggregatorV3Interface.sol:4:pragma solidity 0.6.11;
./Dependencies/BaseMath.sol:2:pragma solidity 0.6.11;
./Dependencies/CheckContract.sol:3:pragma solidity 0.6.11;
./Dependencies/LiquityBase.sol:3:pragma solidity 0.6.11;
./Dependencies/LiquityMath.sol:3:pragma solidity 0.6.11;
./Dependencies/LiquitySafeMath128.sol:3:pragma solidity 0.6.11;
./Dependencies/Ownable.sol:3:pragma solidity 0.6.11;
./Dependencies/SafeMath.sol:3:pragma solidity 0.6.11;
./Dependencies/TellorCaller.sol:3:pragma solidity 0.6.11;
./Dependencies/TroveManagerBase.sol:3:pragma solidity 0.6.11;
./Dependencies/Whitelist.sol:3:pragma solidity 0.6.11;
./Dependencies/YetiCustomBase.sol:3:pragma solidity 0.6.11;
./GasPool.sol:3:pragma solidity 0.6.11;
./HintHelpers.sol:3:pragma solidity 0.6.11;
./Interfaces/IActivePool.sol:3:pragma solidity 0.6.11;
./Interfaces/IBaseOracle.sol:3:pragma solidity 0.6.11;
./Interfaces/IBorrowerOperations.sol:3:pragma solidity 0.6.11;
./Interfaces/ICollateralReceiver.sol:3:pragma solidity 0.6.11;
./Interfaces/ICollSurplusPool.sol:3:pragma solidity 0.6.11;
./Interfaces/ICommunityIssuance.sol:3:pragma solidity 0.6.11;
./Interfaces/IDefaultPool.sol:3:pragma solidity 0.6.11;
./Interfaces/IERC20.sol:3:pragma solidity 0.6.11;
./Interfaces/IERC2612.sol:3:pragma solidity 0.6.11;
./Interfaces/IJoeZapper.sol:3:pragma solidity 0.6.11;
./Interfaces/ILiquityBase.sol:3:pragma solidity 0.6.11;
./Interfaces/ILockupContractFactory.sol:3:pragma solidity 0.6.11;
./Interfaces/ILQTYStaking.sol:3:pragma solidity 0.6.11;
./Interfaces/IOracle.sol:3:pragma solidity 0.6.11;
./Interfaces/IPool.sol:3:pragma solidity 0.6.11;
./Interfaces/IPriceCurve.sol:3:pragma solidity 0.6.11;
./Interfaces/IPriceFeed.sol:3:pragma solidity 0.6.11;
./Interfaces/ISortedTroves.sol:3:pragma solidity 0.6.11;
./Interfaces/IStabilityPool.sol:3:pragma solidity 0.6.11;
./Interfaces/ISYETI.sol:3:pragma solidity 0.6.11;
./Interfaces/ITellor.sol:2:pragma solidity 0.6.11;
./Interfaces/ITellorCaller.sol:3:pragma solidity 0.6.11;
./Interfaces/ITraderJoeZap.sol:3:pragma solidity 0.6.11;
./Interfaces/ITroveManager.sol:3:pragma solidity 0.6.11;
./Interfaces/ITroveManagerLiquidations.sol:3:pragma solidity 0.6.11;
./Interfaces/ITroveManagerRedemptions.sol:3:pragma solidity 0.6.11;
./Interfaces/IUniswapV2Pair.sol:3:pragma solidity >=0.5.0;
./Interfaces/IWAsset.sol:3:pragma solidity 0.6.11;
./Interfaces/IWhitelist.sol:3:pragma solidity 0.6.11;
./Interfaces/IYetiRouter.sol:3:pragma solidity 0.6.11;
./Interfaces/IYETIToken.sol:3:pragma solidity 0.6.11;
./Interfaces/IYUSDToken.sol:3:pragma solidity 0.6.11;
./LPRewards/Dependencies/Address.sol:3:pragma solidity 0.6.11;
./LPRewards/Dependencies/SafeERC20.sol:3:pragma solidity 0.6.11;
./LPRewards/Interfaces/ILPTokenWrapper.sol:3:pragma solidity 0.6.11;
./LPRewards/Interfaces/IUnipool.sol:3:pragma solidity 0.6.11;
./LPRewards/Pool2Unipool.sol:3:pragma solidity 0.6.11;
./LPRewards/TestContracts/ERC20Mock.sol:3:pragma solidity 0.6.11;
./LPRewards/Unipool.sol:3:pragma solidity 0.6.11;
./Migrations.sol:3:pragma solidity 0.6.11;
./MultiTroveGetter.sol:3:pragma solidity 0.6.11;
./PriceCurves/ThreePieceWiseLinearPriceCurve.sol:3:pragma solidity 0.6.11;
./PriceFeed.sol:3:pragma solidity 0.6.11;
./SortedTroves.sol:3:pragma solidity 0.6.11;
./StabilityPool.sol:3:pragma solidity 0.6.11;
./TeamAllocation.sol:3:pragma solidity 0.6.11;
./TroveManager.sol:3:pragma solidity 0.6.11;
./TroveManagerLiquidations.sol:3:pragma solidity 0.6.11;
./TroveManagerRedemptions.sol:2:pragma solidity 0.6.11;
./YETI/BoringCrypto/BoringBatchable.sol:2:pragma solidity 0.6.12;
./YETI/BoringCrypto/BoringERC20.sol:2:pragma solidity 0.6.12;
./YETI/BoringCrypto/BoringMath.sol:2:pragma solidity 0.6.12;
./YETI/BoringCrypto/BoringOwnable.sol:2:pragma solidity 0.6.12;
./YETI/BoringCrypto/Domain.sol:5:pragma solidity 0.6.12;
./YETI/BoringCrypto/ERC20.sol:2:pragma solidity 0.6.12;
./YETI/BoringCrypto/IERC20.sol:2:pragma solidity 0.6.12;
./YETI/CommunityIssuance.sol:3:pragma solidity 0.6.11;
./YETI/LockupContract.sol:3:pragma solidity 0.6.11;
./YETI/LockupContractFactory.sol:3:pragma solidity 0.6.11;
./YETI/ShortLockupContract.sol:3:pragma solidity 0.6.11;
./YETI/sYETIToken.sol:2:pragma solidity 0.6.12;
./YETI/TeamLockup.sol:3:pragma solidity 0.6.11;
./YETI/YETIToken.sol:3:pragma solidity 0.6.11;
./YetiFinanceTreasury.sol:3:pragma solidity 0.6.11;
./YUSDToken.sol:3:pragma solidity 0.6.11;

Tools Used

VS Code

Recommended Mitigation Steps

Consider upgrading pragma to at least 0.8.4.

Handle

Dravee

Vulnerability details

Impact

Explicit initialization with zero is not required for variable declaration because uints are 0 by default. Removing this will reduce contract size and save a bit of gas.

Proof of Concept

Instances:

ActivePool.sol:
  133:         for (uint i = 0; i < poolColl.tokens.length; i++) {
  167:         for (uint i = 0; i < _tokens.length; i++) {
  184:         for (uint i = 0; i < _tokens.length; i++) {

BorrowerOperations.sol:
   699:         for (uint256 i = 0; i < _tokensIn.length; i++) {
   737:         for (uint256 i = 0; i < len; i++) {
   873:         for (uint256 i = 0; i < _colls.length; i++) {
   890:         for (uint256 i = 0; i < arr.length; i++) {
   920:         for (uint256 i = 0; i < _colls1.length; i++) {
   921:             for (uint256 j = 0; j < _colls2.length; j++) {
   929:         for (uint256 i = 0; i < _colls.length; i++) {
  1061:         for (uint i = 0; i < _routers.length; i++) {
  1068:         for (uint i = 0; i < _indices.length - 1; i++) {

DefaultPool.sol:
   97:         for (uint256 i = 0; i < poolColl.tokens.length; i++) {
  133:         for (uint256 i = 0; i < _tokens.length; i++) {

HintHelpers.sol:
  139:         for (uint256 i = 0; i < colls.tokens.length; i++) {

MultiTroveGetter.sol:
   73:         for (uint idx = 0; idx < _startIdx; ++idx) {
   79:         for (uint idx = 0; idx < _count; ++idx) {
   90:         for (uint idx = 0; idx < _startIdx; ++idx) {
   96:         for (uint idx = 0; idx < _count; ++idx) {
  110:         for (uint i = 0; i < data.allColls.length; i++) {

StabilityPool.sol:
   562:         for (uint256 i = 0; i < _amountsAdded.length; i++) {
   588:         for (uint256 i = 0; i < _amountsAdded.length; i++) {
   592:         for (uint256 i = 0; i < _amountsAdded.length; i++) {
   628:         for (uint256 i = 0; i < _assets.length; i++) {
   720:         for (uint256 i = 0; i < assets.length; i++) {
   942:         for (uint256 i = 0; i < assets.length; i++) {
   994:             for (uint256 i = 0; i < colls.length; ++i) {
  1011:         for (uint256 i = 0; i < allColls.length; i++) {

TeamAllocation.sol:
  66:         for (uint i = 0; i < 7; i++) {

TroveManager.sol:
  234:         for (uint i = 0; i < _lowerHints.length; i++) {
  348:         for (uint i = 0; i < allColls.length; i++) {
  374:         for (uint i = 0; i < allColls.length; i++ ) {
  397:         for (uint i = 0; i < allColls.length; i++ ) {
  420:         for (uint i = 0; i < assets.length; i++) {
  460:         for (uint i = 0; i < borrowerColls.length; i++) {
  476:         for (uint i = 0; i < Troves[_borrower].colls.tokens.length; i++) {
  525:         for (uint i = 0; i < _tokens.length; i++) {
  582:         for (uint i = 0; i < allColls.length; i++) {
  603:         for (uint i = 0; i < _tokens.length; i++) {

TroveManagerLiquidations.sol:
  394:         for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
  475:         for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
  701:             for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
  721:             for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
  808:         for (uint i = 0; i < _troveTokens.length; i++) {
  840:         for (uint i = 0; i < _colls.tokens.length; i++) {

TroveManagerRedemptions.sol:
  304:         for (uint256 i = 0; i < colls.tokens.length; i++) {
  367:             for (uint256 i = 0; i < colls.tokens.length; i++) {
  516:         uint256 total = 0;
  517:         for (uint256 i = 0; i < coll.amounts.length; i++) {

Dependencies\LiquityBase.sol:
   34:     // uint constant public MIN_NET_DEBT = 0; 
   63:         for (uint i = 0; i < _coll.tokens.length; i++) {
   97:         for (uint i = 0; i < _tokens.length; i++) {
  106:         for (uint i = 0; i < _colls.tokens.length; i++) {
  115:         for (uint i = 0; i < _colls.tokens.length; i++) {
  149:         for (uint i = 0; i < tokens.length; i++) {
  158:         for (uint i = 0; i < coll.tokens.length; i++) {
  168:         for (uint i = 0; i < _coll.tokens.length; i++) {

Dependencies\YetiCustomBase.sol:
   35:         uint256 n = 0;
   36:         for (uint256 i = 0; i < _coll1.tokens.length; i++) {
   44:         for (uint256 i = 0; i < _coll2.tokens.length; i++) {
   56:         uint256 j = 0;
   59:         for (uint256 i = 0; i < coll3.tokens.length; i++) {
  104:         for (uint256 i = 0; i < _tokens.length; i++) {
  122:         for (uint256 i = 0; i < _subTokens.length; i++) {
  142:         uint256 n = 0;
  144:         for (uint256 i = 0; i < _coll1.tokens.length; i++) {
  152:         for (uint256 i = 0; i < _tokens.length; i++) {
  163:         uint256 j = 0;
  165:         for (uint256 i = 0; i < coll3.tokens.length; i++) {
  178:         for (uint i = 0; i < _arr.length; i++) {

LPRewards\Pool2Unipool.sol:
  80:     uint256 public periodFinish = 0;
  81:     uint256 public rewardRate = 0;

LPRewards\Unipool.sol:
  80:     uint256 public periodFinish = 0;
  81:     uint256 public rewardRate = 0;

YETI\BoringCrypto\BoringBatchable.sol:
  37:         for (uint256 i = 0; i < calls.length; i++) {

YETI\BoringCrypto\BoringERC20.sol:
  19:             uint8 i = 0;

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove explicit initialization with zero.

Handle

Dravee

Vulnerability details

Impact

Checking non-zero transfer values can avoid an external call to save gas.

Proof of Concept

Instances missing a non-zero check:

ActivePool.sol:
  156:         bool sent = IERC20(_collateral).transfer(_to, _amount);

BorrowerOperations.sol:
  742:             bool transferredToActivePool = coll.transferFrom(_from, address(activePool), amount);

DefaultPool.sol:
  121:         bool success = IERC20(_collateral).transfer(activePool, _amount);

StabilityPool.sol:
  947:                 IERC20(assets[i]).transfer(_to, amounts[i]);

TeamAllocation.sol:
  69:             require(YETI.transfer(member, amount));
  77:         YETI.transfer(_to, _amount);

YetiFinanceTreasury.sol:
  25:         _token.transfer(_to, _amount);

AssetWrappers\WJLP\WJLP.sol:
  127:         JLP.transferFrom(_from, address(this), _amount);
  166:         JLP.transfer(_to, _amount);
  273:             JOE.transfer(_to, _amount);

Dependencies\LiquityBase.sol:
  170:             if (!token.transfer(_to, _coll.amounts[i])) {

LPRewards\Dependencies\SafeERC20.sol:
  23:         _callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
  27:         _callOptionalReturn(token, abi.encodeWithSelector(token.transferFrom.selector, from, to, value));

YETI\CommunityIssuance.sol:
  125:         yetiToken.transfer(_account, _YETIamount);

YETI\LockupContract.sol:
  68:         yetiTokenCached.transfer(beneficiary, YETIBalance);

YETI\ShortLockupContract.sol:
  67:         yetiTokenCached.transfer(beneficiary, YETIBalance);

YETI\sYETIToken.sol:
  203:         yetiToken.transfer(to, amount);

YETI\TeamLockup.sol:
  47:             require(YETI.transfer(multisig, _amount));

Tools Used

VS Code

Recommended Mitigation Steps

Check if transfer amount > 0. It is done at some places already, like here: https://github.com/code-423n4/2021-12-yetifinance/blob/1da782328ce4067f9654c3594a34014b0329130a/packages/contracts/contracts/LPRewards/Unipool.sol#L189-L192

Handle

Dravee

Vulnerability details

Impact

In view and pure functions, != 0 costs less gass compared to > 0 for unsigned integer

Proof of Concept

0 checks inside view or pure functions:

BorrowerOperations.sol:
  913:         require(_YUSDChange > 0, "BorrowerOps: Debt increase requires non-zero debtChange");

HintHelpers.sol:
  100:         while (currentTroveuser != address(0) && remainingYUSD > 0 && _maxIterations-- > 0) {

StabilityPool.sol:
  1090:         require(_initialDeposit > 0, "StabilityPool: User must have a non-zero deposit");
  1099:         require(_amount > 0, "StabilityPool: Amount must be non-zero");

TroveManager.sol:
  502:             assert(totalStakesSnapshot[token] > 0);

TroveManagerLiquidations.sol:
  657:         if (_YUSDInStabPool > 0) {

TroveManagerRedemptions.sol:
  497:         require(_amount > 0, "TroveManager: Amount must be greater than zero");
  520:         require(total > 0, "must be non zero redemption amount");

AssetWrappers\WJLP\SafeMath.sol:
  196:         require(b > 0, errorMessage);
  222:         require(b > 0, errorMessage);

Dependencies\CheckContract.sol:
  17:         require(size > 0, "Account code size cannot be zero");

Dependencies\LiquityBase.sol:
  159:             if (coll.amounts[i] > 0) {

Dependencies\LiquityMath.sol:
  83:         if (_debt > 0) {

Dependencies\SafeMath.sol:
  122:         require(b > 0, errorMessage);

Dependencies\TellorCaller.sol:
  50:         if (_value > 0) return (true, _value, _time);

Dependencies\YetiCustomBase.sol:
   38:             if (_coll1.amounts[i] > 0) {
   46:             if (_coll2.amounts[i] > 0) {
   60:             if (coll3.amounts[i] > 0) {
  145:             if (_coll1.amounts[i] > 0) {
  166:             if (coll3.amounts[i] > 0) {

LPRewards\Dependencies\Address.sol:
   34:         return size > 0;
  152:             if (returndata.length > 0) {

PriceCurves\ThreePieceWiseLinearPriceCurve.sol:
  144:         if (decay > 0 && decay < decayTime) {

YETI\sYETIToken.sol:
  271:         require(b > 0, "BoringMath: Div By 0");

Tools Used

VS Code

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

Dravee

Vulnerability details

Impact

In a number of places a keccak("string") expression is assigned to a constant variable. Due to how constant variables are implemented this results in the hash being recomputed each time that the variable is used, spending the gas necessary to perform this action.

If these variables were to be immutable this hash is calculated once at deploy time and then the result is saved to be used directly at runtime rather than recalculating, saving the cost of hashing.

See: ethereum/solidity#9232

Proof of Concept

YETI\YETIToken.sol:
  50:     bytes32 private constant _PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
  51:     bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

YETI\BoringCrypto\Domain.sol:
  10:     bytes32 private constant DOMAIN_SEPARATOR_SIGNATURE_HASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");

Tools Used

VS Code

Recommended Mitigation Steps

Change all constant hashes to be immutable

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

./ActivePool.sol:133:        for (uint i = 0; i < poolColl.tokens.length; i++) {
./ActivePool.sol:167:        for (uint i = 0; i < _tokens.length; i++) {
./ActivePool.sol:184:        for (uint i = 0; i < _tokens.length; i++) {
./BorrowerOperations.sol:699:        for (uint256 i = 0; i < _tokensIn.length; i++) {
./BorrowerOperations.sol:873:        for (uint256 i = 0; i < _colls.length; i++) {
./BorrowerOperations.sol:890:        for (uint256 i = 0; i < arr.length; i++) {
./BorrowerOperations.sol:920:        for (uint256 i = 0; i < _colls1.length; i++) {
./BorrowerOperations.sol:921:            for (uint256 j = 0; j < _colls2.length; j++) {
./BorrowerOperations.sol:929:        for (uint256 i = 0; i < _colls.length; i++) {
./BorrowerOperations.sol:930:            for (uint256 j = i + 1; j < _colls.length; j++) {
./BorrowerOperations.sol:1061:        for (uint i = 0; i < _routers.length; i++) {
./BorrowerOperations.sol:1068:        for (uint i = 0; i < _indices.length - 1; i++) {
./DefaultPool.sol:97:        for (uint256 i = 0; i < poolColl.tokens.length; i++) {
./DefaultPool.sol:133:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/LiquityBase.sol:63:        for (uint i = 0; i < _coll.tokens.length; i++) {
./Dependencies/LiquityBase.sol:97:        for (uint i = 0; i < _tokens.length; i++) {
./Dependencies/LiquityBase.sol:106:        for (uint i = 0; i < _colls.tokens.length; i++) {
./Dependencies/LiquityBase.sol:115:        for (uint i = 0; i < _colls.tokens.length; i++) {
./Dependencies/LiquityBase.sol:149:        for (uint i = 0; i < tokens.length; i++) {
./Dependencies/LiquityBase.sol:158:        for (uint i = 0; i < coll.tokens.length; i++) {
./Dependencies/LiquityBase.sol:168:        for (uint i = 0; i < _coll.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:36:        for (uint256 i = 0; i < _coll1.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:44:        for (uint256 i = 0; i < _coll2.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:59:        for (uint256 i = 0; i < coll3.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:104:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:122:        for (uint256 i = 0; i < _subTokens.length; i++) {
./Dependencies/YetiCustomBase.sol:144:        for (uint256 i = 0; i < _coll1.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:152:        for (uint256 i = 0; i < _tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:165:        for (uint256 i = 0; i < coll3.tokens.length; i++) {
./Dependencies/YetiCustomBase.sol:178:        for (uint i = 0; i < _arr.length; i++) {
./HintHelpers.sol:139:        for (uint256 i = 0; i < colls.tokens.length; i++) {
./MultiTroveGetter.sol:110:        for (uint i = 0; i < data.allColls.length; i++) {
./StabilityPool.sol:562:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:588:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:592:        for (uint256 i = 0; i < _amountsAdded.length; i++) {
./StabilityPool.sol:628:        for (uint256 i = 0; i < _assets.length; i++) {
./StabilityPool.sol:720:        for (uint256 i = 0; i < assets.length; i++) {
./StabilityPool.sol:942:        for (uint256 i = 0; i < assets.length; i++) {
./StabilityPool.sol:994:            for (uint256 i = 0; i < colls.length; ++i) {
./StabilityPool.sol:1011:        for (uint256 i = 0; i < allColls.length; i++) {
./TroveManager.sol:234:        for (uint i = 0; i < _lowerHints.length; i++) {
./TroveManager.sol:348:        for (uint i = 0; i < allColls.length; i++) {
./TroveManager.sol:374:        for (uint i = 0; i < allColls.length; i++ ) {
./TroveManager.sol:397:        for (uint i = 0; i < allColls.length; i++ ) {
./TroveManager.sol:420:        for (uint i = 0; i < assets.length; i++) {
./TroveManager.sol:460:        for (uint i = 0; i < borrowerColls.length; i++) {
./TroveManager.sol:476:        for (uint i = 0; i < Troves[_borrower].colls.tokens.length; i++) {
./TroveManager.sol:525:        for (uint i = 0; i < _tokens.length; i++) {
./TroveManager.sol:582:        for (uint i = 0; i < allColls.length; i++) {
./TroveManager.sol:603:        for (uint i = 0; i < _tokens.length; i++) {
./TroveManagerLiquidations.sol:255:        for (vars.i = 0; vars.i < _troveArray.length; vars.i++) {
./TroveManagerLiquidations.sol:334:        for (vars.i = 0; vars.i < _troveArray.length; vars.i++) {
./TroveManagerLiquidations.sol:394:        for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:475:        for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:701:            for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:721:            for (uint256 i = 0; i < _collsToLiquidate.tokens.length; i++) {
./TroveManagerLiquidations.sol:808:        for (uint i = 0; i < _troveTokens.length; i++) {
./TroveManagerLiquidations.sol:840:        for (uint i = 0; i < _colls.tokens.length; i++) {
./TroveManagerRedemptions.sol:304:        for (uint256 i = 0; i < colls.tokens.length; i++) {
./TroveManagerRedemptions.sol:367:            for (uint256 i = 0; i < colls.tokens.length; i++) {
./TroveManagerRedemptions.sol:517:        for (uint256 i = 0; i < coll.amounts.length; i++) {
./YETI/BoringCrypto/BoringBatchable.sol:37:        for (uint256 i = 0; i < calls.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Store the array's length in a variable before the for-loop, and use it instead.

Handle

Dravee

Vulnerability details

Impact

From the Solidity doc:

If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

Why do Solidity examples use bytes32 type instead of string?

bytes32 uses less gas because it fits in a single word of the EVM, and string is a dynamically sized-type which has current limitations in Solidity (such as can't be returned from a function to a contract).

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Proof of Concept

Instances of string constant that can be replaced by bytes1 constant to bytes32 constant :

./ActivePool.sol:28:    string constant public NAME = "ActivePool";
./HintHelpers.sol:18:    string constant public NAME = "HintHelpers";
./LPRewards/Pool2Unipool.sol:75:    string constant public NAME = "Pool2Unipool";
./LPRewards/Unipool.sol:75:    string constant public NAME = "Unipool";
./PriceFeed.sol:26:    string constant public NAME = "PriceFeed";
./SortedTroves.sol:53:    string constant public NAME = "SortedTroves";
./TroveManager.sol:25:    string constant public NAME = "TroveManager";
./YETI/CommunityIssuance.sol:19:    string constant public NAME = "CommunityIssuance";
./YETI/LockupContract.sol:23:    string constant public NAME = "LockupContract";
./YETI/LockupContractFactory.sol:29:    string constant public NAME = "LockupContractFactory";
./YETI/ShortLockupContract.sol:23:    string constant public NAME = "LockupContract";
./YETI/YETIToken.sol:39:    string constant internal _NAME = "Yeti Finance";
./YETI/YETIToken.sol:40:    string constant internal _SYMBOL = "YETI";
./YETI/YETIToken.sol:41:    string constant internal _VERSION = "1";
./YUSDToken.sol:31:    string constant internal _NAME = "YUSD Stablecoin";
./YUSDToken.sol:32:    string constant internal _SYMBOL = "YUSD";
./YUSDToken.sol:33:    string constant internal _VERSION = "1";

Tools Used

VS Code

Recommended Mitigation Steps

Consider replacing constant string variables with bytes1 to bytes32 variables.