Golom contest - sashik_eth's results

L01 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables. Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

contracts/core/GolomTrader.sol:454  function executeSetDistributor() external onlyOwner { 

contracts/governance/GolomToken.sol:65  function executeSetMinter() external onlyOwner {

contracts/rewards/RewardDistributor.sol:290 function executeChangeTrader() external onlyOwner {

contracts/rewards/RewardDistributor.sol:307 function executeAddVoteEscrow() external onlyOwner { 

L02 - Should use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

contracts/rewards/RewardDistributor.sol:151 rewardToken.transfer(addr, reward);
contracts/rewards/RewardDistributor.sol:165 rewardToken.transfer(addr, reward); 
contracts/rewards/RewardDistributor.sol:207 rewardToken.transfer(tokenowner, reward);
contracts/rewards/RewardDistributor.sol:208 weth.transfer(tokenowner, rewardEth);

N01 - Missing visibility

contracts/core/GolomTrader.sol:45   ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

N02 - Floating pragma is used

The contracts should be deployed with the same solidity version with which they have been tested. Locking the pragma ensures that the contracts don't get deployed with an older version which might introduce bugs.

contracts/governance/GolomToken.sol:2   pragma solidity ^0.8.11;

N03 - Typos

contracts/rewards/RewardDistributor.sol:62 mapping(uint256 => uint256) public rewardExchange; // reward minted each epoc for exhange contracts/rewards/RewardDistributor.sol:107 // this assumes atleast 1 trade is done daily?????? contracts/rewards/RewardDistributor.sol:111 // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining //

N04 - Missing revert string

contracts/rewards/RewardDistributor.sol:88  require(msg.sender == trader);

contracts/rewards/RewardDistributor.sol:144 require(epochs[index] < epoch); 

contracts/rewards/RewardDistributor.sol:158 require(epochs[index] < epoch);

N05 - No needed return

contracts/rewards/RewardDistributor.sol:137 return;

N06 - Missing SPDX license identifier

contracts/vote-escrow/TokenUriHelper.sol:1  /// [MIT License] 

N07 - Variable has constant-style naming

Could cause misleading

contracts/vote-escrow/VoteEscrowDelegation.sol:50   uint256 public MIN_VOTING_POWER_REQUIRED = 0;

N08 - Commented code

contracts/vote-escrow/VoteEscrowDelegation.sol:219  // function removeDelegationByOwner(uint256 delegatedTokenId, uint256 ownerTokenId) external {

N09 - Assert should be used only for testing code

contracts/vote-escrow/VoteEscrowCore.sol:977    assert(_isApprovedOrOwner(msg.sender, _tokenId));
contracts/vote-escrow/VoteEscrowCore.sol:981    assert(_value > 0);
contracts/vote-escrow/VoteEscrowCore.sol:991    assert(_isApprovedOrOwner(msg.sender, _tokenId));

G01 - Variable that never used

contracts/core/GolomTrader.sol:72   address public governance;

G02 - Caching storage variables

Since reading from memory is much cheaper than reading from storage, state variables that are called more than 1 SLOAD inside the function should be cached.

contracts/core/GolomTrader.sol:383  WETH.withdraw(o.totalAmt * amount); // WETH 2nd SLOAD
contracts/core/GolomTrader.sol:402  distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee); // distributor 2nd SLOAD

contracts/rewards/RewardDistributor.sol:132 emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee); // epoch 5 SLOADs during this logic block
contracts/rewards/RewardDistributor.sol:136 epochTotalFee[epoch] = epochTotalFee[epoch] + fee; // epoch 6 SLOADs during this logic block
contracts/rewards/RewardDistributor.sol:191 ve.totalSupplyAt(epochBeginTime[1]); // 2 SLOADs ve, 2 SLOADs epochBeginTime during this logic block
contracts/rewards/RewardDistributor.sol:202 ve.totalSupplyAt(epochBeginTime[epochs[index]]); // 4 SLOADs ve, 4 SLOADs epochBeginTime during this logic block
contracts/rewards/RewardDistributor.sol:225 for (uint256 index = 0; index < epoch; index++) { // 2 SLOADs epoch
contracts/rewards/RewardDistributor.sol:233 ve.totalSupplyAt(epochBeginTime[1]); // 2 SLOADs ve, 2 SLOADs epochBeginTime during this logic block
contracts/rewards/RewardDistributor.sol:244 ve.totalSupplyAt(epochBeginTime[index]); // 4 SLOADs ve, 4 SLOADs epochBeginTime during this logic block
contracts/rewards/RewardDistributor.sol:227 if (claimed[tokenid][index] == 0){ // claimed[tokenid][index] 2nd SLOAD

G03 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

contracts/vote-escrow/VoteEscrowDelegation.sol:79   Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1]; // could be unchecked due to check on previous line
contracts/vote-escrow/VoteEscrowDelegation.sol:119  return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray; // could be unchecked due to statement logic
contracts/vote-escrow/VoteEscrowDelegation.sol:138  if (checkpoints[nftId][nCheckpoints - 1].fromBlock <= blockNumber) { // could be unchecked due to check on line 133
contracts/vote-escrow/VoteEscrowDelegation.sol:150  uint256 center = upper - (upper - lower) / 2; // could be unchecked due to check on previous line
contracts/vote-escrow/VoteEscrowDelegation.sol:157  upper = center - 1;  // could be unchecked due to function logic

contracts/vote-escrow/VoteEscrowCore.sol:1166   uint256 t_i = (last_point.ts / WEEK) * WEEK; // could be unchecked since constant have non-zero value 
contracts/vote-escrow/VoteEscrowCore.sol:1124   _max = _mid - 1; // could be unchecked due to loop logic
contracts/vote-escrow/VoteEscrowCore.sol:1053   _max = _mid - 1; // could be unchecked due to loop logic
contracts/vote-escrow/VoteEscrowCore.sol:744    uint256 t_i = (last_checkpoint / WEEK) * WEEK; // could be unchecked since constant have non-zero value 

contracts/rewards/RewardDistributor.sol:285 traderEnableDate = block.timestamp + 1 days; // could be unchecked since overflow will be in extreme far furute

contracts/core/GolomTrader.sol:449  distributorEnableDate = block.timestamp + 1 days; // could be unchecked since overflow will be in extreme far furute

contracts/governance/GolomToken.sol:60  minterEnableDate = block.timestamp + 1 days; // could be unchecked since overflow will be in extreme far furute

G04 - Variable that should be immutable since it never changes after setting in constructor

contracts/rewards/RewardDistributor.sol:46  uint256 public startTime;

contracts/rewards/RewardDistributor.sol:68  ERC20 public rewardToken;
contracts/rewards/RewardDistributor.sol:69  ERC20 public weth; 

G05 - Too long revert string

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Also using custom errors could save some gas: https://blog.soliditylang.org/2021/04/21/custom-errors/

contracts/governance/GolomToken.sol:24  require(msg.sender == minter, 'GolomToken: only reward distributor can enable'); 
contracts/rewards/RewardDistributor.sol:180 require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
contracts/rewards/RewardDistributor.sol:291 require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
contracts/rewards/RewardDistributor.sol:308 require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet'); 
contracts/vote-escrow/VoteEscrowCore.sol:608    revert('ERC721: transfer to non ERC721Receiver implementer');
contracts/vote-escrow/VoteEscrowCore.sol:929    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
contracts/vote-escrow/VoteEscrowCore.sol:945    require(unlock_time > block.timestamp, 'Can only lock until time in the future');
contracts/vote-escrow/VoteEscrowCore.sol:983    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
contracts/vote-escrow/VoteEscrowDelegation.sol:73   require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

G06 - Optimised for loops

Loop could save gas if:

1. Cache condition value
2. Use prefix ++i increment
3. Put increment inside unchecked block It would have next view:

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

Next loops could be optimized:

contracts/core/GolomTrader.sol:415  for (uint256 i = 0; i < proof.length; i++) {

contracts/rewards/RewardDistributor.sol:143 for (uint256 index = 0; index < epochs.length; index++) {
contracts/rewards/RewardDistributor.sol:157 for (uint256 index = 0; index < epochs.length; index++) { 
contracts/rewards/RewardDistributor.sol:225 for (uint256 index = 0; index < epoch; index++) {

contracts/rewards/RewardDistributor.sol:257 for (uint256 index = 0; index < epoch; index++) { 
contracts/rewards/RewardDistributor.sol:272 for (uint256 index = 0; index < epoch; index++) { 

contracts/vote-escrow/VoteEscrowCore.sol:745    for (uint256 i = 0; i < 255; ++i) { 
contracts/vote-escrow/VoteEscrowCore.sol:1044   for (uint256 i = 0; i < 128; ++i) {
contracts/vote-escrow/VoteEscrowCore.sol:1115   for (uint256 i = 0; i < 128; ++i) {
contracts/vote-escrow/VoteEscrowCore.sol:1167   for (uint256 i = 0; i < 255; ++i) { 

contracts/vote-escrow/VoteEscrowDelegation.sol:171  for (uint256 index = 0; index < delegated.length; index++) {
contracts/vote-escrow/VoteEscrowDelegation.sol:189  for (uint256 index = 0; index < delegatednft.length; index++) { 
contracts/vote-escrow/VoteEscrowDelegation.sol:199  for (uint256 i; i < _array.length; i++) { 

G07 - Comparison > 0 is less gas efficient than != 0 with uint256 in require statement with optimizer

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

G08 - Cache result of call to same function that does not change during execution

rewardToken.balanceOf(address(ve)) called 2 times, rewardToken.totalSupply() called 3 times:

contracts/rewards/RewardDistributor.sol:112 uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) / rewardToken.totalSupply();
contracts/rewards/RewardDistributor.sol:114 uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();