Golom contest - robee's results

Duplicates in array

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.
    

Code instance:

VoteEscrowDelegation.delegate pushes (tokenId)

Mult instead div in compares

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

Code instances:

    GolomTrader.sol, 211, require( o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000, 'amt not matching' ); 

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    RewardDistributor.addFee (fee)
    DummyRewardDistributor.addFee (fee)

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instance:

    Deprecated safeApprove in GolomAirdrop.sol line 94: token.safeApprove(_ve, type(uint256).max);

Require with empty message

The following requires are with empty messages. This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: RewardDistributor.sol, In line 158 with Empty Require message.
    Solidity file: VoteEscrowCore.sol, In line 944 with Empty Require message.
    Solidity file: VoteEscrowCore.sol, In line 895 with Empty Require message.
    Solidity file: GolomTrader.sol, In line 350 with Empty Require message.
    

Require with not comprehensive message

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

Code instances:

    Solidity file: GolomTrader.sol, In line 217 with Require message: mgmtm

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    GolomToken.sol.setMinter _minter
    RewardDistributor.sol.traderClaim addr
    

Hardcoded WETH

WETH address is hardcoded but it may differ on other chains, e.g. Polygon, so make sure to check this before deploying and update if necessary You should consider injecting WETH address via the constructor. (previous issue: https://github.com/code-423n4/2021-10-ambire-findings/issues/54)

Code instance:

    Hardcoded weth address in GolomTrader.sol

Assert instead require to validate user inputs

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    

Code instances:

    VoteEscrowCore.sol : reachable assert in line 860
    VoteEscrowCore.sol : reachable assert in line 518
    VoteEscrowCore.sol : reachable assert in line 665
    VoteEscrowCore.sol : reachable assert in line 1205
    VoteEscrowCore.sol : reachable assert in line 492
    VoteEscrowCore.sol : reachable assert in line 990

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

Code instance:

    GolomAirdrop.sol, updateEndTimestamp

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    DummyRewardDistributor.sol: function addFee parameter fee isn't used. (addFee is public)
    DummyRewardDistributor.sol: function addFee parameter addr isn't used. (addFee is public)

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L682
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/core/GolomTrader.sol#L154
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/rewards/GolomAirdrop.sol#L203
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L861
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/rewards/DummyRewardDistributor.sol#L44

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/governance/GolomToken.sol#L58
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/rewards/GolomAirdrop.sol#L142
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/governance/Timlock.sol#L55
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L868
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/governance/Timlock.sol#L72
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L664
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/core/GolomTrader.sol#L444

Must approve 0 first

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code instance:

approve without approving 0 first GolomAirdrop.sol, 94, token.safeApprove(_ve, type(uint256).max);

Div by 0

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

Code instances:

    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/rewards/RewardDistributor.sol#L195 epochs might be 0
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L1219 point might be 0

Tokens with fee on transfer are not supported

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can be lower for those tokens. Therefore it's recommended to use the balance change before and after the transfer instead of the amount. This way you also support the tokens with transfer fee - that are popular.

Code instance:

    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L861

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):

Code instances:

    RewardDistributor.sol, 209 (multiStakerClaim), weth.transfer(tokenowner, rewardEth);
    RewardDistributor.sol, 208 (multiStakerClaim), rewardToken.transfer(tokenowner, reward);
    RewardDistributor.sol, 165 (exchangeClaim), rewardToken.transfer(addr, reward);
    GolomTrader.sol, 301 (fillBid), nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
    RewardDistributor.sol, 151 (traderClaim), rewardToken.transfer(addr, reward);
    GolomTrader.sol, 154 (payEther), payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
    GolomTrader.sol, 361 (fillCriteriaBid), nftcontract.transferFrom(msg.sender, o.signer, tokenId);

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instances:

    trader in DummyRewardDistributor.sol
    eip712DomainHash in Emitter.sol

Unused state variables

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

Code instances:

    VoteEscrowCore.sol, decimals
    RewardDistributor.sol, rewardLP
    VoteEscrowCore.sol, version
    RewardDistributor.sol, claimedUpto
    VoteEscrowCore.sol, ERC165_INTERFACE_ID
    VoteEscrowCore.sol, ERC721_METADATA_INTERFACE_ID
    VoteEscrowCore.sol, ERC721_INTERFACE_ID

Caching array length can save gas

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

Code instances:

    RewardDistributor.sol, epochs, 157
    GolomTrader.sol, proof, 415
    VoteEscrowDelegation.sol, delegated, 171
    VoteEscrowDelegation.sol, _array, 199
    RewardDistributor.sol, tokenids, 180
    VoteEscrowDelegation.sol, delegatednft, 189
    RewardDistributor.sol, epochs, 143

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: VoteEscrowDelegation.sol, i, 199
    just change to unchecked: VoteEscrowCore.sol, i, 1044
    just change to unchecked: VoteEscrowCore.sol, i, 1167
    change to prefix increment and unchecked: GolomTrader.sol, i, 415
    change to prefix increment and unchecked: VoteEscrowDelegation.sol, index, 171
    change to prefix increment and unchecked: RewardDistributor.sol, index, 226
    change to prefix increment and unchecked: RewardDistributor.sol, index, 143
    change to prefix increment and unchecked: RewardDistributor.sol, index, 157
    change to prefix increment and unchecked: VoteEscrowDelegation.sol, index, 189
    change to prefix increment and unchecked: RewardDistributor.sol, tindex, 180
    change to prefix increment and unchecked: RewardDistributor.sol, index, 273
    just change to unchecked: VoteEscrowCore.sol, i, 1115
    change to prefix increment and unchecked: RewardDistributor.sol, index, 258

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    RewardDistributor.sol, 157
    VoteEscrowCore.sol, 1044
    VoteEscrowCore.sol, 1167
    VoteEscrowDelegation.sol, 189
    VoteEscrowCore.sol, 1115
    VoteEscrowDelegation.sol, 171
    RewardDistributor.sol, 273
    RewardDistributor.sol, 143
    RewardDistributor.sol, 258
    RewardDistributor.sol, 180
    RewardDistributor.sol, 226
    GolomTrader.sol, 415

Unnecessary default assignment

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

Code instances:

    RewardDistributor.sol (L#45) : uint256 public epoch = 0;
    VoteEscrowDelegation.sol (L#50) : uint256 public MIN_VOTING_POWER_REQUIRED = 0; 

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instance:

In VoteEscrowCore.sol,rearranging the storage fields can optimize to: 12 slots from: 14 slots. The new order of types (you choose the actual variables): 1. uint256 2. uint256 3. uint256 4. uint256 5. uint256 6. string 7. string 8. string 9. uint256 10. address 11. bytes4 12. bytes4 13. bytes4 14. address 15. uint8 16. uint8 17. uint8 18. uint8 19. int128

Use bytes32 instead of string to save gas whenever possible

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.

Code instance:

    TokenUriHelper.sol (L106), string memory json = Base64.encode( bytes( string( abi.encodePacked( '{"name": "Lock #', toString(_tokenId), '", "description": "Golom locks, can be used to claim protocol fees and token emission", "attributes": [{"trait_type":"Lock End","value":"', toString(_locked_end), 

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instances:

    Solidity file: RewardDistributor.sol, In line 292, Require message length to shorten: 36, The message: RewardDistributor: time not over yet
    Solidity file: VoteEscrowCore.sol, In line 983, Require message length to shorten: 36, The message: Cannot add to expired lock. Withdraw
    Solidity file: VoteEscrowCore.sol, In line 929, Require message length to shorten: 36, The message: Cannot add to expired lock. Withdraw
    Solidity file: RewardDistributor.sol, In line 309, Require message length to shorten: 36, The message: RewardDistributor: time not over yet
    Solidity file: VoteEscrowCore.sol, In line 945, Require message length to shorten: 38, The message: Can only lock until time in the future
    Solidity file: VoteEscrowDelegation.sol, In line 73, Require message length to shorten: 36, The message: VEDelegation: Need more voting power

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    VoteEscrowCore.sol, 981: change '_value > 0' to '_value != 0'
    VoteEscrowCore.sol, 727: change 'epoch > 0' to 'epoch != 0'
    VoteEscrowDelegation.sol, 103: change 'nCheckpoints > 0' to 'nCheckpoints != 0'
    VoteEscrowCore.sol, 944: change '_value > 0' to '_value != 0'
    VoteEscrowCore.sol, 927: change '_value > 0' to '_value != 0'
    VoteEscrowDelegation.sol, 73: change 'balance > 0' to 'balance != 0'
    GolomTrader.sol, 152: change 'payAmt > 0' to 'payAmt != 0'

Unnecessary cast

Code instances:

    Order GolomTrader.sol.cancelOrder - unnecessary casting Order(o)
    Order GolomTrader.sol.fillBid - unnecessary casting Order(o)
    Order GolomTrader.sol.fillAsk - unnecessary casting Order(o)
    Order GolomTrader.sol.validateOrder - unnecessary casting Order(o)
    Order GolomTrader.sol.fillCriteriaBid - unnecessary casting Order(o)
    Order Emitter.sol.pushOrder - unnecessary casting Order(o)

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instances:

    VoteEscrowCore.sol (L#942) - uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; 
    RewardDistributor.sol (L#286) - traderEnableDate = block.timestamp + 1 days;
    RewardDistributor.sol (L#302) - voteEscrowEnableDate = block.timestamp + 1 days;
    GolomTrader.sol (L#449) - distributorEnableDate = block.timestamp + 1 days;
    VoteEscrowCore.sol (L#999) - require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max'); 
    VoteEscrowCore.sol (L#946) - require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max'); ++tokenId;
    GolomToken.sol (L#60) - minterEnableDate = block.timestamp + 1 days;
    VoteEscrowCore.sol (L#994) - uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; 
    RewardDistributor.sol (L#106) - if (block.timestamp > startTime + (epoch) * secsInDay) { 
    GolomAirdrop.sol (L#160) - require(block.timestamp + 30 days > newEndTimestamp, 'Owner: New timestamp too far');

Empty else statement can be removed to save gas

    Empty else statement can be removed to save gas.
    

Code instance:

    RewardDistributor.sol.addFee

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    VoteEscrowCore.sol, _balance, { return ownerToNFTokenCount[_owner]; }
    GolomTrader.sol, _hashOrder, { return _hashOrderinternal(o, [o.nonce, o.deadline]); }
    Emitter.sol, hashOrder, { return _hashOrderinternal(o, [o.nonce, o.deadline]); }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    VoteEscrowCore.sol, _balanceOfAtNFT
    Emitter.sol, hashOrder
    Emitter.sol, _hashOrderinternal
    VoteEscrowCore.sol, _isContract
    VoteEscrowCore.sol, _mint
    VoteEscrowCore.sol, _addTokenToOwnerList
    GolomTrader.sol, _hashOrderinternal
    VoteEscrowCore.sol, _clearApproval
    VoteEscrowCore.sol, _removeTokenFromOwnerList
    Emitter.sol, emitOrder
    GolomTrader.sol, _hashOrder

Cache powers of 10 used several times

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

Code instance:

RewardDistributor.sol, 99 : You should cache the used power of 10 as constant state variable since it's used several times (2): if (rewardToken.totalSupply() > 1000000000 * 10**18) {

Change if -> revert pattern to require

Change if -> revert pattern to 'require' to save gas and improve code quality, if (some_condition) { revert(revert_message) }

to: require(!some_condition, revert_message)

In the following locations:

Code instance:

    GolomTrader.sol, 426

Do not cache msg.sender

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

Code instances:

    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/core/GolomTrader.sol#L225
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/governance/Timlock.sol#L66
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L54
    https://github.com/code-423n4/2022-07-golom/tree/main/contracts/vote-escrow/VoteEscrowCore.sol#L855