Golom contest - Kaiziron's results

Use != 0 instead of > 0 when comparing unsigned integers

!= 0 will do the same as > 0 for unsigned integers, but != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled.

Instances include :

vote-escrow/VoteEscrowCore.sol:927:        require(_value > 0); // dev: need non-zero value
vote-escrow/VoteEscrowCore.sol:928:        require(_locked.amount > 0, 'No existing lock found');
vote-escrow/VoteEscrowCore.sol:944:        require(_value > 0); // dev: need non-zero value
vote-escrow/VoteEscrowCore.sol:982:        require(_locked.amount > 0, 'No existing lock found');
vote-escrow/VoteEscrowCore.sol:997:        require(_locked.amount > 0, 'Nothing is locked');

Recommendation

It is recommended to replace > 0 with != 0, as they do the same thing for unsigned integers, and '!= 0' costs less gas compared to > 0 in require statements with the optimizer enabled, also enable the optimizer.

For example :

vote-escrow/VoteEscrowCore.sol:927:        require(_value != 0); // dev: need non-zero value

---

Don't explicitly initialize variables with the default value

Uninitialized variables are assigned with the default value of their type, initializing a variable with its default value costs unnecessary gas.

Instances include :

rewards/RewardDistributor.sol:45:    uint256 public epoch = 0;
rewards/RewardDistributor.sol:142:        uint256 reward = 0;
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:156:        uint256 reward = 0;
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:175:        uint256 reward = 0;
rewards/RewardDistributor.sol:176:        uint256 rewardEth = 0;
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:222:        uint256 reward = 0;
rewards/RewardDistributor.sol:223:        uint256 rewardEth = 0;
rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:257:        uint256 reward = 0;
rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:272:        uint256 reward = 0;
rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
vote-escrow/VoteEscrowDelegation.sol:50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;
vote-escrow/VoteEscrowDelegation.sol:147:        uint256 lower = 0;
vote-escrow/VoteEscrowDelegation.sol:170:        uint256 votes = 0;
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:188:        uint256 votes = 0;
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowCore.sol:735:        uint256 block_slope = 0; // dblock/dt
vote-escrow/VoteEscrowCore.sol:745:            for (uint256 i = 0; i < 255; ++i) {
vote-escrow/VoteEscrowCore.sol:1042:        uint256 _min = 0;
vote-escrow/VoteEscrowCore.sol:1044:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1113:        uint256 _min = 0;
vote-escrow/VoteEscrowCore.sol:1115:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1133:        uint256 d_block = 0;
vote-escrow/VoteEscrowCore.sol:1134:        uint256 d_t = 0;
vote-escrow/VoteEscrowCore.sol:1167:        for (uint256 i = 0; i < 255; ++i) {
vote-escrow/VoteEscrowCore.sol:1211:        uint256 dt = 0;

Recommendation

It is recommended to initialize variables without assigning them the default value, for example :

rewards/RewardDistributor.sol:45:    uint256 public epoch;

---

Cache array length outside of for loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Instances include :

rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

Recommendation

It is recommended to cache the array length on a variable before running the loop, then it doesn't need to read the length on every iteration, which cost gas, for example :

uint256 len = proposal.targets.length;
for (uint256 i = 0; i < len; i++) {

---

If possible, use bitwise shift instead of division/multiplication

If the divisor/multiplier x is a power of 2, it can be calculated by shifting log2(x) to the right/left. Division with / cost more gas compared to bitwise shifting.

Instances include :

vote-escrow/VoteEscrowDelegation.sol:150:            uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow
vote-escrow/VoteEscrowCore.sol:1049:            uint256 _mid = (_min + _max + 1) / 2;
vote-escrow/VoteEscrowCore.sol:1120:            uint256 _mid = (_min + _max + 1) / 2;

Recommendation

It is recommended to replace / and * with >> and << respectively and divisor/multiplier x to log2(x), for division/multiplication where the divisor/multiplier is a power of 2, for example :

vote-escrow/VoteEscrowCore.sol:1049:            uint256 _mid = (_min + _max + 1) >> 1;

---

If possible, use prefix increment instead of postfix increment

Prefix increment ++i returns the updated value after it's incremented and postfix increment i++ returns the original value then increments it. Prefix increment costs less gas compared to postfix increment.

Instances includes :

rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
vote-escrow/TokenUriHelper.sol:138:            digits++;

Recommendation

It is recommended to use prefix increment instead of postfix one when the return value is not needed, as both of them will give the same result and prefix increment costs less gas.

For example :

vote-escrow/TokenUriHelper.sol:138:            ++digits;

---

Don't use long revert strings

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Instances includes :

governance/GolomToken.sol:24:        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
rewards/RewardDistributor.sol:181:            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
rewards/RewardDistributor.sol:292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
rewards/RewardDistributor.sol:309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
vote-escrow/VoteEscrowDelegation.sol:73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
vote-escrow/VoteEscrowDelegation.sol:130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
vote-escrow/VoteEscrowDelegation.sol:186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
vote-escrow/VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
vote-escrow/VoteEscrowCore.sol:945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
vote-escrow/VoteEscrowCore.sol:946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
vote-escrow/VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
vote-escrow/VoteEscrowCore.sol:1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

Recommendation

It is recommended to use error code and providing a reference to the error code instead of a long revert string., for example :

// Link to the reference of error codes
require(condition, "ERR");

---

Public function could be declared external

Public functions that are never called by the contract should be declared external to save gas.

Instances includes :

fillBid(GolomTrader.Order,uint256,address,GolomTrader.Payment) should be declared external:
	- GolomTrader.fillBid(GolomTrader.Order,uint256,address,GolomTrader.Payment) (contracts/core/GolomTrader.sol#279-308)
cancelOrder(GolomTrader.Order) should be declared external:
	- GolomTrader.cancelOrder(GolomTrader.Order) (contracts/core/GolomTrader.sol#312-317)
fillCriteriaBid(GolomTrader.Order,uint256,uint256,bytes32[],address,GolomTrader.Payment) should be declared external:
	- GolomTrader.fillCriteriaBid(GolomTrader.Order,uint256,uint256,bytes32[],address,GolomTrader.Payment) (contracts/core/GolomTrader.sol#334-368)
addFee(address[2],uint256) should be declared external:
	- DummyRewardDistributor.addFee(address[2],uint256) (contracts/rewards/DummyRewardDistributor.sol#37)
addFee(address[2],uint256) should be declared external:
	- RewardDistributor.addFee(address[2],uint256) (contracts/rewards/RewardDistributor.sol#98-138)
traderClaim(address,uint256[]) should be declared external:
	- RewardDistributor.traderClaim(address,uint256[]) (contracts/rewards/RewardDistributor.sol#141-152)
exchangeClaim(address,uint256[]) should be declared external:
	- RewardDistributor.exchangeClaim(address,uint256[]) (contracts/rewards/RewardDistributor.sol#155-166)
multiStakerClaim(uint256[],uint256[]) should be declared external:
	- RewardDistributor.multiStakerClaim(uint256[],uint256[]) (contracts/rewards/RewardDistributor.sol#172-210)
stakerRewards(uint256) should be declared external:
	- RewardDistributor.stakerRewards(uint256) (contracts/rewards/RewardDistributor.sol#215-250)
traderRewards(address) should be declared external:
	- RewardDistributor.traderRewards(address) (contracts/rewards/RewardDistributor.sol#254-265)
exchangeRewards(address) should be declared external:
	- RewardDistributor.exchangeRewards(address) (contracts/rewards/RewardDistributor.sol#269-280)
_tokenURI(uint256,uint256,uint256,uint256) should be declared external:
	- TokenUriHelper._tokenURI(uint256,uint256,uint256,uint256) (contracts/vote-escrow/TokenUriHelper.sol#66-126)
getPriorVotes(uint256,uint256) should be declared external:
	- VoteEscrow.getPriorVotes(uint256,uint256) (contracts/vote-escrow/VoteEscrowDelegation.sol#185-193)

Recommendation

Use the external attribute for functions never called from the contract.

---