Golom contest - NoamYakov's results

[1] Local variables shadows state variables in VoteEscrowDelegation.sol

The contract VoteEscrow inherits the state variables tokenId and checkpoint from the contract VoteEscrowCore. However, these state variables shadowed by local variables that use these exact names.

Recommendation

Rename those local variables to avoid unintentional programming errors and to improve code readability.

Code References

• VoteEscrow.delegate(): tokenId and checkpoint
• VoteEscrow._getCurrentDelegated(): tokenId
• VoteEscrow.getVotes(): tokenId
• VoteEscrow.getPriorVotes(): tokenId
• VoteEscrow.removeDelegation(): tokenIdandcheckpoint`

[2] Division by zero

Division by zero causes a revert without any informative error message (it raises a Panic(0x12) exception).

Recommendation

Check that a variable is non-zero before using it as a divisor.

Code References

• RewardDistributor.sol: lines 147-148 (epochTotalFee[epochs[index]]), 161-162 (epochTotalFee[epochs[index]]), 191-192 (ve.totalSupplyAt(epochBeginTime[1])), 197-198 (ve.totalSupplyAt(epochBeginTime[epochs[index]])), 202-203 (ve.totalSupplyAt(epochBeginTime[epochs[index]])), 233-234 (ve.totalSupplyAt(epochBeginTime[1])), 239-240 (ve.totalSupplyAt(epochBeginTime[index])), 244-245 (ve.totalSupplyAt(epochBeginTime[index])), 261-262 (epochTotalFee[index]), 276-277 (epochTotalFee[index])

[3] Array out-of-bounds

Array out-of-bounds access causes a revert without any informative error message (it raises a Panic(0x32) exception).

Recommendation

Check that an index which is used to access an array element fits the array's boundaries before accessing that array element.

Code References

• VoteEscrowCore.sol: line 385 (idx must be < 1000000000)
• RewardDistributor.sol: line 177 (tokenids.length must be > 0)

[4] Missing non-zero tests for parameters in setter functions

Functions with the purpose of setting or resetting state variables can be invoked by mistake and cause some serious problems, sometimes irreversible.

Recommendation

In order to significantly reduce the chances for this kind of human error, it is recommended to check that the parameters passed to the setter function have non-zero values.

Code References

• GolomToken.sol: setMinter()
• RewardDistributor.sol: changeTrader(), addVoteEscrow()
• GolomTrader.sol: setDistributor()

[5] Missing check of IERC20.transferFrom()'s return value for WETH token

The IERC20.transferFrom() function should return a boolean indicating whether the transfer succeeded or not. However, that return value isn't always tested for the WETH token in GolomTrader.sol. This issue look harmless at first, because WETH's transferFrom() function is implemented such that it always return true, and reverts on failures. Yet, it is still dangerous to live this behavior because future version of WETH that are going to be used by future versions of GolomTrader may return false on failure.

Recommendation

Check the return value of the IERC20.transferFrom() every time it is called, and handle the case of failure. Another (better) option is to use OpenZeppelin's SafeERC20 library, specifically the SafeERC20.safeTransferFrom() function.

Code References

• GolomTrader.sol: line 382

[1] Unnecessary MLOADs and SLOADs in for-each loops

There are many for loops that follows this for-each pattern:

for (uint256 i = 0; i < array.length; i++) {
        // do something with `array[i]`
}

In such for loops, the array.length is read on every iteration, instead of caching it once in a local variable and read it from there. Storage reads are much more expensive than reading local variables. Memory reads are a bit more expensive than reading local variables.

Recommendation

Read these values from storage or memory once, cache them in local variables and then read them again from the local variables. For example:

uint256 length = array.length;
for (uint256 i = 0; i < length; i++) {
        // do something with `array[i]`
}

Code References

• GolomTrader.sol: line 415
• RewardDistributor.sol: lines 143, 157, 180, 183
• VoteEscrowDelegation.sol: lines 171, 189, 199

[2] Default value initialization

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc. depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example:

uint256 x = 0;

can be optimized to:

uint256 x;

Code References

• GolomTrader.sol: line 415
• RewardDistributor.sol: lines 45, 142, 143, 156, 157, 175, 176, 180, 183, 222, 223, 226, 257, 258, 272, 273
• VoteEscrowCore.sol: lines 697, 698, 735, 745, 749, 1042, 1044, 1113, 1115, 1133, 1134, 1167, 1169, 1211
• VoteEscrowDelegation.sol: lines 50, 147, 170, 171, 188, 189

[3] ++ and -- are cheaper than += 1, x = x + 1, -= 1 and x = x - 1

Use prefix increments (++x) and decrements (--x) instead of increments by 1 (x += 1 and x = x + 1) and decrements by 1 (x -= 1 and x = x - 1).

Recommendation

Change all increments by 1 to prefix increments and change all decrements by 1 to prefix decrements.

Code References

• RewardDistributor.sol: line 118
• TokenUriHelper.sol: line 143
• VoteEscrowCore.sol: lines 499, 512, 768, 885, 890

[4] Use if instead of else if in VoteEscrowDelegation._getPriorDelegated()

if statements are cheaper than else if statements. Therefore, the code

if (cp.fromBlock == blockNumber) {
    return cp.delegatedTokenIds;
} else if (cp.fromBlock < blockNumber) {
    lower = center;
} else {
    upper = center - 1;
}

can be optimized to

if (cp.fromBlock == blockNumber) {
    return cp.delegatedTokenIds;
}
if (cp.fromBlock < blockNumber) {
    lower = center;
} else { // Still executed only when `cp.fromBlock > blockNumber`.
    upper = center - 1;
}

[5] Unnecessary array boundaries check when loading an array element twice

Some functions loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using that local variable, skipping the redundant second array boundaries check and saving gas.

Recommendation

Load the array elements once, cache them in local variables and then read them again using the local variables. For example:

uint256 item = array[i];
// do something with `item`
// do some other thing with `item`

Code References

• RewardDistributor.sol: lines 144+147+148+149 (epochs[index]), 158+161+162+163 (epochs[index]), 181+185+186+191+197+202 (tokenids[tindex]), 184+185+186+187+197+198+201+202+203 (epochs[index])

[6] public functions not called by the contract should be declared external instead

external functions are cheaper than public function.

Recommendation

Re-declare public functions as external functions when they aren't called by the contract.

Code References

• GolomTrader.sol: lines 209 (fillAsk), 284 (fillBid), 312 (cancelOrder), 341 (fillCriteriaBid)
• RewardDistributor.sol: lines 98 (addFee), 141 (traderClaim), 155 (exchangeClaim), 172 (multiStakerClaim), 215 (stakerRewards), 254 (traderRewards), 269 (exchangeRewards)
• TokenUriHelper.sol: line 71 (_tokenURI)
• VoteEscrowDelegation.sol: line 185 (getPriorVotes)

[7] Prefix increments are cheaper than postfix increments

Use prefix increments (++x) instead of postfix increments (x++).

Recommendation

Change all postfix increments to prefix increments.

Code References

• GolomTrader.sol: line 415
• RewardDistributor.sol: lines 143, 157, 180, 183, 226, 258, 273
• TokenUriHelper.sol: line 138
• VoteEscrowDelegation.sol: lines 171, 189, 199

[8] Unnecessary checked arithmetic in for loops

There is no risk of overflow caused by increments to the iteration index in for loops (the i++ in for (uint256 i = 0; i < numIterations; i++)). Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the loop

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

to

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

It is a little less readable but it saves a significant amount of gas.

Code References

• GolomTrader.sol: line 415
• RewardDistributor.sol: lines 143, 157, 180, 183, 226, 258, 273
• VoteEscrowCore.sol: lines 745, 1044, 1115, 1167
• VoteEscrowDelegation.sol: lines 171, 189, 199

[9] Unnecessary SLOADs

Reading variables that use storage as their memory location is an expensive process. Much more expensive than reading local variables.

Recommendation

When such a variable is read more than once, cache it in a local variable to avoid more SLOADs.

Code References

• VoteEscrowDelegation.sol: lines 199+201 (_array.length)
• RewardDistributor.sol: lines 100+112+113+114+122 (rewardToken), 106+117+118 (epoch), 112+114 (ve), 117+136 (epochTotalFee[epoch]), 119+120+121+123+125+132+134+135+136 (epoch), 173+177+181+191+192+197+198+202+203 (ve), 191+192 (epochBeginTime[1]), 197+198+202+203 (epochBeginTime[epochs[index]]), 220+233+234+239+240+244+245 (ve), 224+226 (epoch), 233+234 (epochBeginTime[1]), 239+240+244+245 (epochBeginTime[index]), 258 (epoch), 273 (epoch)
• GolomTrader.sol: lines 189+193 (filled[hashStruct]), 382+383 (WETH), 384+402 (distributor), 644+650 (idToOwner[_tokenId])

[10] Use of immutable state variables

Reading immutable state variables isn't as expensive as reading mutable (regular) state variables.

Recommendation

Use immutable state variables for every state variable that never changes its value after initialization.

Code References

• RewardDistributor.sol: lines 46 (startTime), 68 (rewardToken), 69 (weth)
• VoteEscrowCore.sol: line 300 (token - initialized in VoteEscrow's constructor)

[11] Use of constants state variables

Reading constants state variables isn't as expensive as reading any other type of state variables (mutable or immutable).

Recommendation

Use constant state variables for every state variable that has a constant value.

Code References

• GolomTrader.sol: line 45 (WETH)
• RewardDistributor.sol: line 46 (startTime)

[12] Unnecessary external call

External calls are expensive and should not take place more than once if they don't have any side-effects (i.e., if they are calls to view functions).

Recommendation

execute the external call once, cache the return value in a local variable and then use that local variable instead of executing the external call again. For example:

uint256 result = someContract.someViewFuction();
// do something with `result`
// do some other thing with `result`

Code References

• RewardDistributor.sol: lines 112+113+114 (rewardToken.totalSupply()), 112+114 (rewardToken.balanceOf(address(ve))), 197+202 (ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])), 198+203 (ve.totalSupplyAt(epochBeginTime[epochs[index]])), 239+244 (ve.balanceOfAtNFT(tokenid, epochBeginTime[index])), 240+245 (ve.totalSupplyAt(epochBeginTime[index]))

[13] Inlining private and internal functions

private and internal functions only called once can be inlined to save gas.

Recommendation

Inline private and internal function only called once.

Code References

• VoteEscrowCore.sol: lines 497 (_addTokenToOwnerList()), 510 (_removeTokenFromOwnerList()), 542 (_clearApproval()), 602 (_isContract()), 950 (_mint()), 1157 (_balanceOfAtNFT())
• VoteEscrowDelegation.sol: lines 169 (_getCurrentDelegated()), 187 (_getPriorDelegated()), 214 (removeElement())
• GolomTrader.sol: lines 124 (_hashOrderinternal()), 174 (_hashOrder())

[14] Addition to and subtraction from state variables

x += y and x -= y cost more gas than x = x + y and x = x - y for state variables.

Recommendation

Use x = x + y and x = x - y instead of x += y and x -= y for state variables.

Code References

• VoteEscrowCore.sol: lines 499, 512

[15] Non-zero tests for unsigned integers

Testing != 0 is cheaper than testing > 0 for unsigned integers.

Recommendation

Use != 0 instead of > 0 when testing unsigned integers.

Code References

• GolomTrader.sol: lines 152, 250, 387
• RewardDistributor.sol: line 124
• VoteEscrowCore.sol: lines 579, 727, 927, 944, 981
• VoteEscrowDelegation.sol: lines 78, 103, 119

[16] Long require()/revert() strings

require()/revert() strings longer than 32 bytes cost extra gas.

Recommendation

Use require()/revert() strings no longer than 32 bytes.

Code References

• GolomToken.sol: line 24
• RewardDistributor.sol: lines 181, 292, 309
• VoteEscrowCore.sol: lines 608, 929, 945, 983
• VoteEscrowDelegation.sol: lines 73

[17] Unnecessary checked arithmetic

Checked arithmetic costs more gas than unchecked arithmetic.

Recommendation

Use unchecked arithmetic (unchecked {} blocks) when there is no risk of overflow and underflow.

Code References

• GolomTrader.sol: lines 193 (o.tokenAmt - filled[hashStruct]), 324 (++nonces[msg.sender]), 449 (block.timestamp + 1 days)
• GolomToken.sol: line 60 (block.timestamp + 1 days)
• RewardDistributor.sol: lines 106 (startTime + (epoch) * secsInDay), 112 (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve))), 114 (tokenToEmit * rewardToken.balanceOf(address(ve))), 118 (epoch + 1), 121 (tokenToEmit - stakerReward), 286 (block.timestamp + 1 days), 302 (block.timestamp + 1 days)
• TokenUriHelper.sol: lines 17 (4 * ((len + 2) / 3)), 20 (encodedLen + 32), 143 (digits -= 1), 144 (48 + uint256(value % 10))
• VoteEscrowCore.sol: lines 706 (old_locked.end - block.timestamp), 710 (new_locked.end - block.timestamp), 737 (block.timestamp - last_point.ts), 744 ((last_checkpoint / WEEK) * WEEK), 818 (user_point_epoch[_tokenId] + 1), 865 (supply_before + _value), 885 (attachments[_tokenId] + 1), 942 ((.../* must use checked arithmetic! */...) * WEEK), 946 (block.timestamp + MAXTIME), 994 ((.../* must use checked arithmetic! */...) * WEEK), 999 (block.timestamp + MAXTIME), 1029 (supply_before - value), 1053 (_mid - 1), 1124 (_mid - 1), 1166 ((last_point.ts / WEEK) * WEEK)
• VoteEscrowDelegation.sol: lines 79 (nCheckpoints - 1), 138 (nCheckpoints - 1), 139 (nCheckpoints - 1), 148 (nCheckpoints - 1), 150 (upper - (upper - lower) / 2), 201 (_array.length - 1)

[18] Unnecessary if statement in VoteEscrowDelegation._writeCheckpoint()

The code

Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
    ...
}

can be optimized to

Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

if (oldCheckpoint.fromBlock == block.number) {
    ...
}

skipping the nCheckpoints > 0 test, because if nCheckpoints == 0 than the code would already failed and revert when tring to compute nCheckpoints - 1.

[19] Inefficient storage layout

State variables smaller than 32-bytes can share a storage slot if they both fit in a 32-byte slot.

Recommendation

Whenever possible, declare these state variable in a way they are packed together. For example: the layout

uint8 x;
uint256 y;
uint8 z;

can be optimized to

uint256 y;
uint8 x;
uint8 z;

where x and z are sharing a single storage slot.

Code References

• VoteEscrowCore.sol: token and _entered_state can be packed together.

[20] Inefficient struct layout

struct fields smaller than 32-bytes can share a storage / memory / calldata slot (depending on their data location) if they both fit in a 32-byte slot.

Recommendation

Whenever possible, declare these storage fields in a way they are packed together. For example: the layout

uint8 x;
uint256 y;
uint8 z;

can be optimized to

uint256 y;
uint8 x;
uint8 z;

where x and z are sharing a single slot.

Code References

• GolomTrader.sol: reservedAddress, isERC721 and v in the Order struct can be packed together.