Sherlock contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {

//Operations not effecting the length of the array.

}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Furthermore, there is no need to assign the initial value 0. This costs extra gas.

Recommended implementation:

uint length = arr.length;

for (uint i; i < length; ++i) {

//Operations not effecting the length of the array.

}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

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

Handle

0x0x0x

Vulnerability details

Concept

In SherDistributionManager.sol#calcReward, slopeRewardsAvailable is not always used. When _amount <= maxRewardsAvailable, this parameter is not required.

slopeRewardsAvailable can before the if-block starting with if (slopeRewardsAvailable != 0) {

This will save gas and make it more readable (atleast for me).

Handle

0x0x0x

Vulnerability details

Concept

Negation(!) is a better optimized implementation of applying == false. It requires less gas and also the recommended approach to compute the negation.

In other words: x == false and !x compute the same result, but !x costs less gas.

Scope

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

Handle

0x0x0x

Vulnerability details

Concept

In https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L366-L375, there is two if-statements to prevent zero addresses.

As can be seen from OZ implementation in https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L336-L337, those zero address checks are already implemented by OZ.

to is directly checked and from is indirectly checked by forcing ownership.