Sherlock contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

++i costs less gas 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:

contracts\managers\Manager.sol:46:    for (uint256 i; i < _extraTokens.length; i++) {
contracts\managers\SherlockClaimManager.sol:234:    for (uint256 i; i < claimCallbacks.length; i++) {
contracts\managers\SherlockClaimManager.sol:505:    for (uint256 i; i < claimCallbacks.length; i++) {
contracts\managers\SherlockProtocolManager.sol:729:    for (uint256 i; i < _protocol.length; i++) {
contracts\SherBuy.sol:186:    for (uint256 i; i < _tokens.length; i++) {
contracts\Sherlock.sol:106:    for (uint256 i; i < _initialstakingPeriods.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable.

Handle

Dravee

Vulnerability details

Impact

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save 77 gas per iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Proof of Concept

Instances include:

contracts\managers\Manager.sol:46:    for (uint256 i; i < _extraTokens.length; i++) {
contracts\managers\SherlockClaimManager.sol:234:    for (uint256 i; i < claimCallbacks.length; i++) {
contracts\managers\SherlockClaimManager.sol:505:    for (uint256 i; i < claimCallbacks.length; i++) {
contracts\managers\SherlockProtocolManager.sol:729:    for (uint256 i; i < _protocol.length; i++) {
contracts\SherBuy.sol:186:    for (uint256 i; i < _tokens.length; i++) {
contracts\Sherlock.sol:106:    for (uint256 i; i < _initialstakingPeriods.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { i++; }  
}  

While the risk are overflow is inexistant for a uint256 i, you might want to keep in mind to manually check for bounds once before the for-loop if i is smaller than a uint256 (such as uint8, but it's never recommended and it's not the case in this project).

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/SherlockClaimManager.sol#L572-L573 There's no readability or gas gain from copying claims_[claimIdentifier] to a variable and then accessing its updated attribute instead of directly using claims_[claimIdentifier].updated.

Tools Used

VS Code

Recommended Mitigation Steps

Do not store this data in a variable

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive (~100 gas) compared to MLOADs (~3 gas). Minimizing them can save gas.

Proof of Concept

In SherlockClaimManager.sol:addCallback(): the code is as such (see @audit-info comments for the opcodes):

File: SherlockClaimManager.sol
225:   function addCallback(ISherlockClaimManagerCallbackReceiver _callback)
226:     external
227:     onlyOwner
228:     nonReentrant
229:   {
230:     if (address(_callback) == address(0)) revert ZeroArgument();
231:     // Checks to see if the max amount of callback contracts has been reached
232:     if (claimCallbacks.length == MAX_CALLBACKS) revert InvalidState(); //@audit-info SLOAD
233:     // Checks to see if this callback contract already exists
234:     for (uint256 i; i < claimCallbacks.length; i++) {  //@audit-info SLOAD (1 per iteration)
235:       if (claimCallbacks[i] == _callback) revert InvalidArgument();
236:     }
237: 
238:     claimCallbacks.push(_callback);
239:     emit CallbackAdded(_callback);
240:   }

The code can be optimized by minimizing the number of SLOADs. Here's what I suggest:

File: SherlockClaimManager.sol
225:   function addCallback(ISherlockClaimManagerCallbackReceiver _callback)
226:     external
227:     onlyOwner
228:     nonReentrant
229:   {
230:     if (address(_callback) == address(0)) revert ZeroArgument();
231:     uint256 claimCallbacksLength = claimCallbacks.length; //@audit-info SLOAD + MSTORE
232:     // Checks to see if the max amount of callback contracts has been reached
233:     if (claimCallbacksLength == MAX_CALLBACKS) revert InvalidState(); //@audit-info MLOAD
234:     // Checks to see if this callback contract already exists
235:     for (uint256 i; i < claimCallbacksLength; i++) {  //@audit-info MLOAD (1 per iteration)
236:       if (claimCallbacks[i] == _callback) revert InvalidArgument();
237:     }
238: 
239:     claimCallbacks.push(_callback);
240:     emit CallbackAdded(_callback);
241:   }

Tools Used

VS Code

Recommended Mitigation Steps

Apply the suggested optimization

Handle

Dravee

Vulnerability details

Impact

!true is cheaper than == false

Proof of Concept

Instances include:

contracts\managers\Manager.sol:52:    if (success == false) revert InvalidConditions();
contracts\managers\SherlockClaimManager.sol:285:    if (_isCleanupState(_oldState) == false) revert 
InvalidState();
contracts\managers\SherlockClaimManager.sol:417:    if (_isEscalateState(_oldState, updated) == false) revert  
InvalidState();
contracts\managers\SherlockClaimManager.sol:502:    if (_isPayoutState(_oldState, updated) == false) revert    
InvalidState();
contracts\managers\SherlockProtocolManager.sol:677:    if (able == false) revert InvalidConditions();
contracts\SherBuy.sol:89:    if (_sherlockPosition.stakingPeriods(PERIOD) == false) revert InvalidState();     
contracts\SherBuy.sol:126:    if (active() == false) revert InvalidState();
contracts\SherClaim.sol:79:    if (active() == false) revert InvalidState();

Tools Used

VS Code

Recommended Mitigation Steps

Replace comparisons to the boolean constant false with the NOT operator:

contracts\managers\Manager.sol:52:    if (!success) revert InvalidConditions();
contracts\managers\SherlockClaimManager.sol:285:    if (!_isCleanupState(_oldState)) revert 
InvalidState();
contracts\managers\SherlockClaimManager.sol:417:    if (!_isEscalateState(_oldState, updated)) revert  
InvalidState();
contracts\managers\SherlockClaimManager.sol:502:    if (!_isPayoutState(_oldState, updated)) revert    
InvalidState();
contracts\managers\SherlockProtocolManager.sol:677:    if (!able) revert InvalidConditions();
contracts\SherBuy.sol:89:    if (!_sherlockPosition.stakingPeriods(PERIOD)) revert InvalidState();     
contracts\SherBuy.sol:126:    if (!active()) revert InvalidState();
contracts\SherClaim.sol:79:    if (!active()) revert InvalidState();

Handle

Dravee

Vulnerability details

Impact

Reducing from public to private or internal will save gas

Proof of Concept

contracts\managers\SherlockClaimManager.sol:31:  uint256 public constant ESCALATE_TIME = 4 weeks;
contracts\managers\SherlockClaimManager.sol:37:  uint256 public constant UMAHO_TIME = 24 hours;
contracts\managers\SherlockClaimManager.sol:41:  uint256 public constant SPCC_TIME = 7 days;
contracts\managers\SherlockClaimManager.sol:50:  bytes32 public constant override UMA_IDENTIFIER =
contracts\managers\SherlockClaimManager.sol:53:  uint256 public constant MAX_CALLBACKS = 4;
contracts\managers\SherlockClaimManager.sol:56:  SkinnyOptimisticOracleInterface public constant UMA =
contracts\managers\SherlockClaimManager.sol:60:  IERC20 public constant TOKEN = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
contracts\managers\SherlockProtocolManager.sol:23:  uint256 public constant MIN_BALANCE_SANITY_CEILING = 30_000 * 10**6; // 30k usdc
contracts\managers\SherlockProtocolManager.sol:26:  uint256 public constant PROTOCOL_CLAIM_DEADLINE = 7 days;
contracts\managers\SherlockProtocolManager.sol:29:  uint256 public constant MIN_SECONDS_LEFT = 7 days;
contracts\managers\SherlockProtocolManager.sol:36:  uint256 public constant MIN_SECONDS_OF_COVERAGE = 12 hours;
contracts\SherBuy.sol:36:  uint256 public constant PERIOD = 26 weeks;
contracts\Sherlock.sol:25:  uint256 public constant ARB_RESTAKE_WAIT_TIME = 2 weeks;
contracts\Sherlock.sol:28:  uint256 public constant ARB_RESTAKE_GROWTH_TIME = 1 weeks;
contracts\Sherlock.sol:31:  uint256 public constant ARB_RESTAKE_PERIOD = 12 weeks;
contracts\Sherlock.sol:34:  uint256 public constant ARB_RESTAKE_MAX_PERCENTAGE = (10**18 / 100) * 20; // 20%

Tools Used

VS Code

Recommended Mitigation Steps

Consider making some constants as non-public to save gas

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

In SherlockProtocolManager.sol:_activeBalance(), the code is as follows:

File: SherlockProtocolManager.sol
214:   function _activeBalance(bytes32 _protocol) internal view returns (uint256) {
215:     uint256 debt = _calcIncrementalProtocolDebt(_protocol);
216:     uint256 balance = activeBalances[_protocol];
217:     // The debt should never be higher than the balance (only happens if the arbitrages fail)
218:     if (debt > balance) return 0;
219:     return balance - debt;
220:   }

However, this can be optimized :

• Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO)
• In this case here, if debt == balance, 0 should be returned
• Avoiding the else clause would avoid some opcodes (1 SUB, 2 MLOADs...)

The optimized code would become:

File: SherlockProtocolManager.sol
214:   function _activeBalance(bytes32 _protocol) internal view returns (uint256) {
215:     uint256 debt = _calcIncrementalProtocolDebt(_protocol);
216:     uint256 balance = activeBalances[_protocol];
217:     // The debt should never be higher than the balance (only happens if the arbitrages fail)
218:     if (debt >= balance) return 0;
219:     return balance - debt;
220:   }

Tools Used

VS Code

Recommended Mitigation Steps

Use the non-strict greater-than operator >= in this particular case instead of the strict one >

Handle

Dravee

Vulnerability details

Impact

Due to how constant variables are implemented, an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Proof of Concept

contracts\managers\SherDistributionManager.sol:23:  uint256 internal constant DECIMALS = 10**6;
contracts\managers\SherlockClaimManager.sol:28:  uint256 internal constant BOND = 9_600 * 10**6; // 20k bond
contracts\managers\SherlockProtocolManager.sol:23:  uint256 public constant MIN_BALANCE_SANITY_CEILING = 30_000 * 10**6; // 30k usdc
contracts\managers\SherlockProtocolManager.sol:32:  uint256 internal constant HUNDRED_PERCENT = 10**18;
contracts\SherBuy.sol:38:  uint256 internal constant SHER_STEPS = 10**16;
contracts\SherBuy.sol:40:  uint256 internal constant RATE_STEPS = 10**4;
contracts\SherBuy.sol:42:  uint256 internal constant SHER_DECIMALS = 10**18;
contracts\Sherlock.sol:34:  uint256 public constant ARB_RESTAKE_MAX_PERCENTAGE = (10**18 / 100) * 20; // 20%

Tools Used

VS Code

Recommended Mitigation Steps

Change these expressions from constant to immutable and implement the calculation in the constructor

Handle

Dravee

Vulnerability details

Impact

Strict inequalities add a check of non equality which costs around 3 gas.

Proof of Concept

The following should use an inclusive inequality to save gas:

contracts\managers\SherlockProtocolManager.sol:801:    if (_secondsOfCoverageLeft(_protocol) < MIN_SECONDS_LEFT) revert InsufficientBalance(_protocol);
contracts\SherClaim.sol:42:    if (_claimableAt < block.timestamp + CLAIM_PERIOD_SANITY_BOTTOM) revert InvalidState();
contracts\SherClaim.sol:43:    if (_claimableAt > block.timestamp + CLAIM_PERIOD_SANITY_CEILING) revert InvalidState();
contracts\Sherlock.sol:438:    if (lockupEnd_[_id] > block.timestamp) revert InvalidConditions();

Tools Used

VS Code

Recommended Mitigation Steps

Use >= or <= instead of > and < when possible. Here, it's on time comparisons. As the averable block time in Ethereum is 14 seconds: the gas cost gain from this optimization might be worth the slim difference

Handle

Dravee

Vulnerability details

Impact

Increased gas cost due to unnecessary automatic overflow/underflow checks.

Proof of Concept

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers.

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

This can't get underflowed:

File: Sherlock.sol
463:     if (_amount > mainBalance) {
464:       yieldStrategy.withdraw(_amount - mainBalance); //@audit-info can't underflow
465:     }

Tools Used

VS Code

Recommended Mitigation Steps

Uncheck arithmetic operations when the risk of underflow or overflow is already contained.