Golom contest - oyc_109's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L154

Vulnerability details

Use .call instead of .transfer

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L154

The use of the transfer() function for an address will inevitably make the transaction fail when:

1. The receiver smart contract does not implement a payable function.

2. The receiver smart contract does implement a payable fallback which uses more than 2300 gas unit.

3. The receiver smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Recommended Mitigation Steps

Use call() instead of transfer() to send ETH. And return value must be checked to see if the call is successful. Sending ETH with the transfer is no longer recommended.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L301 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L361

Vulnerability details

use safeTransferFrom for ERC721

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L301 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L361

description

The function exercise is using transferFrom instead of safeTransferFrom function when transferring ERC721 function and if the exerciser is a smart contract and not aware of incoming ERC721 token, the token could be lost forever

recommendation

use safeTransferFrom instead of transferFrom

[L-01] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

GolomToken.sol::2 => pragma solidity ^0.8.11;

[L-02] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

GolomToken.sol::60 => minterEnableDate = block.timestamp + 1 days;
GolomToken.sol::69 => require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');
GolomTrader.sol::182 => if (block.timestamp > o.deadline) {
GolomTrader.sol::449 => distributorEnableDate = block.timestamp + 1 days;
GolomTrader.sol::455 => require(distributorEnableDate <= block.timestamp, 'not allowed');
RewardDistributor.sol::106 => if (block.timestamp > startTime + (epoch) * secsInDay) {
RewardDistributor.sol::286 => traderEnableDate = block.timestamp + 1 days;
RewardDistributor.sol::292 => require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
RewardDistributor.sol::302 => voteEscrowEnableDate = block.timestamp + 1 days;
RewardDistributor.sol::309 => require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
VoteEscrowCore.sol::704 => if (old_locked.end > block.timestamp && old_locked.amount > 0) {
VoteEscrowCore.sol::706 => u_old.bias = u_old.slope * int128(int256(old_locked.end - block.timestamp));
VoteEscrowCore.sol::708 => if (new_locked.end > block.timestamp && new_locked.amount > 0) {
VoteEscrowCore.sol::710 => u_new.bias = u_new.slope * int128(int256(new_locked.end - block.timestamp));
VoteEscrowCore.sol::726 => Point memory last_point = Point({bias: 0, slope: 0, ts: block.timestamp, blk: block.number});
VoteEscrowCore.sol::736 => if (block.timestamp > last_point.ts) {
VoteEscrowCore.sol::737 => block_slope = (MULTIPLIER * (block.number - last_point.blk)) / (block.timestamp - last_point.ts);
VoteEscrowCore.sol::750 => if (t_i > block.timestamp) {
VoteEscrowCore.sol::751 => t_i = block.timestamp;
VoteEscrowCore.sol::769 => if (t_i == block.timestamp) {
VoteEscrowCore.sol::801 => if (old_locked.end > block.timestamp) {
VoteEscrowCore.sol::810 => if (new_locked.end > block.timestamp) {
VoteEscrowCore.sol::821 => u_new.ts = block.timestamp;
VoteEscrowCore.sol::864 => emit Deposit(from, _tokenId, _value, _locked.end, deposit_type, block.timestamp);
VoteEscrowCore.sol::929 => require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
VoteEscrowCore.sol::942 => uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; // Locktime is rounded down to weeks
VoteEscrowCore.sol::945 => require(unlock_time > block.timestamp, 'Can only lock until time in the future');
VoteEscrowCore.sol::946 => require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
VoteEscrowCore.sol::983 => require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
VoteEscrowCore.sol::994 => uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; // Locktime is rounded down to weeks
VoteEscrowCore.sol::996 => require(_locked.end > block.timestamp, 'Lock expired');
VoteEscrowCore.sol::999 => require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
VoteEscrowCore.sol::1011 => require(block.timestamp >= _locked.end, "The lock didn't expire");
VoteEscrowCore.sol::1028 => emit Withdraw(msg.sender, _tokenId, value, block.timestamp);
VoteEscrowCore.sol::1087 => _balanceOfNFT(_tokenId, block.timestamp),
VoteEscrowCore.sol::1095 => return _balanceOfNFT(_tokenId, block.timestamp);
VoteEscrowCore.sol::1141 => d_t = block.timestamp - point_0.ts;
VoteEscrowCore.sol::1199 => return totalSupplyAtT(block.timestamp);
VoteEscrowCore.sol::1219 => dt = ((_block - point.blk) * (block.timestamp - point.ts)) / (block.number - point.blk);
VoteEscrowDelegation.sol::56 => point_history[0].ts = block.timestamp;

[L-03] Unused receive() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

GolomTrader.sol::461 => receive() external payable {}
RewardDistributor.sol::315 => receive() external payable {}

[L-04] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). Unless there is a compelling reason, abi.encode should be preferred. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

GolomTrader.sol::175 => bytes32 hash = keccak256(abi.encodePacked('\x19\x01', EIP712_DOMAIN_TYPEHASH, hashStruct));

[L-05] require() should be used instead of assert()

require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

VoteEscrowCore.sol::493 => assert(idToOwner[_tokenId] == address(0));
VoteEscrowCore.sol::506 => assert(idToOwner[_tokenId] == _from);
VoteEscrowCore.sol::519 => assert(idToOwner[_tokenId] == _owner);
VoteEscrowCore.sol::666 => assert(_operator != msg.sender);
VoteEscrowCore.sol::679 => assert(_to != address(0));
VoteEscrowCore.sol::861 => assert(IERC20(token).transferFrom(from, address(this), _value));
VoteEscrowCore.sol::977 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::981 => assert(_value > 0); // dev: need non-zero value
VoteEscrowCore.sol::991 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::1007 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::1023 => assert(IERC20(token).transfer(msg.sender, value));
VoteEscrowCore.sol::1110 => assert(_block <= block.number);
VoteEscrowCore.sol::1206 => assert(_block <= block.number);

[L-06] Unsafe use of transfer()/transferFrom() with ERC20/ERC721

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

Using transferFrom for ERC721 could cause NFTs to be stuck in contracts

GolomTrader.sol::236 => ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
GolomTrader.sol::301 => nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
GolomTrader.sol::361 => nftcontract.transferFrom(msg.sender, o.signer, tokenId);
GolomTrader.sol::382 => WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
RewardDistributor.sol::151 => rewardToken.transfer(addr, reward);
RewardDistributor.sol::165 => rewardToken.transfer(addr, reward);
RewardDistributor.sol::208 => rewardToken.transfer(tokenowner, reward);
RewardDistributor.sol::209 => weth.transfer(tokenowner, rewardEth);
VoteEscrowCore.sol::861 => assert(IERC20(token).transferFrom(from, address(this), _value));
VoteEscrowCore.sol::1023 => assert(IERC20(token).transfer(msg.sender, value));

[L-07] Missing checks for zero address

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L29
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L44
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L52
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L53
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L80-L83
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L94
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L870

[L-08] Constants should be declared rather than use magic numbers

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L120-L121 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L212 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L254-L255

[L-09] missing two step ownership change

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L868-L871

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

GolomToken.sol::2 => pragma solidity ^0.8.11;
GolomTrader.sol::3 => pragma solidity 0.8.11;
RewardDistributor.sol::2 => pragma solidity 0.8.11;
TokenUriHelper.sol::3 => pragma solidity 0.8.11;
VoteEscrowCore.sol::2 => pragma solidity 0.8.11;
VoteEscrowDelegation.sol::2 => pragma solidity 0.8.11;

[N-02] Large multiples of ten should use scientific notation

(e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability

GolomTrader.sol::212 => o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
GolomTrader.sol::242 => payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
GolomTrader.sol::255 => 10000 -
GolomTrader.sol::263 => (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
GolomTrader.sol::269 => distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);
GolomTrader.sol::381 => uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
RewardDistributor.sol::100 => if (rewardToken.totalSupply() > 1000000000 * 10**18) {
VoteEscrowCore.sol::308 => mapping(uint256 => Point[1000000000]) public user_point_history; // user -> Point[user_epoch]

[N-03] Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

for better readability

RewardDistributor.sol::48 => uint256 constant dailyEmission = 600000 * 10**18;
RewardDistributor.sol::100 => if (rewardToken.totalSupply() > 1000000000 * 10**18) {

[N-04] require()/revert() statements should have descriptive reason strings

for troubleshotting purposes

GolomTrader.sol::220 => require(msg.sender == o.reservedAddress);
GolomTrader.sol::291 => require(msg.sender == o.reservedAddress);
GolomTrader.sol::293 => require(o.orderType == 1);
GolomTrader.sol::295 => require(status == 3);
GolomTrader.sol::296 => require(amountRemaining >= amount);
GolomTrader.sol::313 => require(o.signer == msg.sender);
GolomTrader.sol::342 => require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
GolomTrader.sol::345 => require(msg.sender == o.reservedAddress);
GolomTrader.sol::347 => require(o.orderType == 2);
GolomTrader.sol::349 => require(status == 3);
GolomTrader.sol::350 => require(amountRemaining >= amount);
GolomTrader.sol::426 => revert('invalid proof');
RewardDistributor.sol::88 => require(msg.sender == trader);
RewardDistributor.sol::144 => require(epochs[index] < epoch);
RewardDistributor.sol::158 => require(epochs[index] < epoch);
VoteEscrowCore.sol::360 => require(_entered_state == _not_entered);
VoteEscrowCore.sol::608 => revert('ERC721: transfer to non ERC721Receiver implementer');
VoteEscrowCore.sol::646 => require(owner != address(0));
VoteEscrowCore.sol::648 => require(_approved != owner);
VoteEscrowCore.sol::652 => require(senderIsOwner || senderIsApprovedForAll);
VoteEscrowCore.sol::869 => require(msg.sender == voter);
VoteEscrowCore.sol::874 => require(msg.sender == voter);
VoteEscrowCore.sol::879 => require(msg.sender == voter);
VoteEscrowCore.sol::884 => require(msg.sender == voter);
VoteEscrowCore.sol::889 => require(msg.sender == voter);
VoteEscrowCore.sol::895 => require(_from != _to);
VoteEscrowCore.sol::927 => require(_value > 0); // dev: need non-zero value
VoteEscrowCore.sol::944 => require(_value > 0); // dev: need non-zero value

[N-05] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

VoteEscrowCore.sol::293 => event Supply(uint256 prevSupply, uint256 supply);

[N-06] Should use time units instead of calculating time base variables (eg. 4 days)

VoteEscrowCore.sol::296 => uint256 internal constant MAXTIME = 4 * 365 * 86400;
VoteEscrowCore.sol::297 => int128 internal constant iMAXTIME = 4 * 365 * 86400;

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
RewardDistributor.sol::142 => uint256 reward = 0;
RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::156 => uint256 reward = 0;
RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::175 => uint256 reward = 0;
RewardDistributor.sol::176 => uint256 rewardEth = 0;
RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::222 => uint256 reward = 0;
RewardDistributor.sol::223 => uint256 rewardEth = 0;
RewardDistributor.sol::226 => for (uint256 index = 0; index < epoch; index++) {
RewardDistributor.sol::257 => uint256 reward = 0;
RewardDistributor.sol::258 => for (uint256 index = 0; index < epoch; index++) {
RewardDistributor.sol::272 => uint256 reward = 0;
RewardDistributor.sol::273 => for (uint256 index = 0; index < epoch; index++) {
VoteEscrowCore.sol::745 => for (uint256 i = 0; i < 255; ++i) {
VoteEscrowCore.sol::1044 => for (uint256 i = 0; i < 128; ++i) {
VoteEscrowCore.sol::1115 => for (uint256 i = 0; i < 128; ++i) {
VoteEscrowCore.sol::1167 => for (uint256 i = 0; i < 255; ++i) {
VoteEscrowCore.sol::1211 => uint256 dt = 0;
VoteEscrowDelegation.sol::147 => uint256 lower = 0;
VoteEscrowDelegation.sol::170 => uint256 votes = 0;
VoteEscrowDelegation.sol::171 => for (uint256 index = 0; index < delegated.length; index++) {
VoteEscrowDelegation.sol::188 => uint256 votes = 0;
VoteEscrowDelegation.sol::189 => for (uint256 index = 0; index < delegatednft.length; index++) {

[G-02] Cache Array Length Outside of 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.

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

[G-03] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0. This change saves 6 gas per instance

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

[G-04] Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

VoteEscrowCore.sol::296 => uint256 internal constant MAXTIME = 4 * 365 * 86400;
VoteEscrowCore.sol::297 => int128 internal constant iMAXTIME = 4 * 365 * 86400;
VoteEscrowCore.sol::1049 => uint256 _mid = (_min + _max + 1) / 2;
VoteEscrowCore.sol::1120 => uint256 _mid = (_min + _max + 1) / 2;
VoteEscrowDelegation.sol::150 => uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

[G-05] Use calldata instead of memory

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

GolomTrader.sol::127 => function _hashOrderinternal(Order calldata o, uint256[2] memory extra) private pure returns (bytes32) {
TokenUriHelper.sol::12 => function encode(bytes memory data) internal pure returns (string memory) {
VoteEscrowCore.sol::1164 => function _supply_at(Point memory point, uint256 t) internal view returns (uint256) {

[G-06] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

GolomTrader.sol::41 => bytes32 public immutable EIP712_DOMAIN_TYPEHASH;

[G-07] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

GolomToken.sol::36 => function mint(address _account, uint256 _amount) external onlyMinter {
GolomToken.sol::42 => function mintAirdrop(address _airdrop) external onlyOwner {
GolomToken.sol::50 => function mintGenesisReward(address _rewardDistributor) external onlyOwner {
GolomToken.sol::58 => function setMinter(address _minter) external onlyOwner {
GolomToken.sol::65 => function executeSetMinter() external onlyOwner {
GolomTrader.sol::444 => function setDistributor(address _distributor) external onlyOwner {
GolomTrader.sol::454 => function executeSetDistributor() external onlyOwner {
RewardDistributor.sol::98 => function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
RewardDistributor.sol::285 => function changeTrader(address _trader) external onlyOwner {
RewardDistributor.sol::291 => function executeChangeTrader() external onlyOwner {
RewardDistributor.sol::298 => function addVoteEscrow(address _voteEscrow) external onlyOwner {
RewardDistributor.sol::308 => function executeAddVoteEscrow() external onlyOwner {
VoteEscrowDelegation.sol::260 => function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {

[G-08] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

GolomTrader.sol::459 => fallback() external payable {}
GolomTrader.sol::461 => receive() external payable {}
RewardDistributor.sol::313 => fallback() external payable {}
RewardDistributor.sol::315 => receive() external payable {}

[G-09] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

GolomTrader.sol::62 => uint8 v;
VoteEscrowCore.sol::261 => int128 bias;
VoteEscrowCore.sol::262 => int128 slope; // # -dweight / dt
VoteEscrowCore.sol::271 => int128 amount;
VoteEscrowCore.sol::297 => int128 internal constant iMAXTIME = 4 * 365 * 86400;
VoteEscrowCore.sol::320 => uint8 public constant decimals = 18;
VoteEscrowCore.sol::356 => uint8 internal constant _not_entered = 1;
VoteEscrowCore.sol::357 => uint8 internal constant _entered = 2;
VoteEscrowCore.sol::358 => uint8 internal _entered_state = 1;
VoteEscrowCore.sol::697 => int128 old_dslope = 0;
VoteEscrowCore.sol::698 => int128 new_dslope = 0;

[G-10] Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. Use uint256(1) and uint256(2) for true/false instead

GolomToken.sol::20 => bool public isAirdropMinted;
GolomToken.sol::21 => bool public isGenesisRewardMinted;
VoteEscrowCore.sol::314 => mapping(uint256 => bool) public voted;

[G-11] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

GolomTrader.sol::324 => uint256 newNonce = ++nonces[msg.sender];
GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) {
RewardDistributor.sol::226 => for (uint256 index = 0; index < epoch; index++) {
RewardDistributor.sol::258 => for (uint256 index = 0; index < epoch; index++) {
RewardDistributor.sol::273 => for (uint256 index = 0; index < epoch; index++) {
TokenUriHelper.sol::138 => digits++;
VoteEscrowCore.sol::745 => for (uint256 i = 0; i < 255; ++i) {
VoteEscrowCore.sol::948 => ++tokenId;
VoteEscrowCore.sol::1044 => for (uint256 i = 0; i < 128; ++i) {
VoteEscrowCore.sol::1115 => for (uint256 i = 0; i < 128; ++i) {
VoteEscrowCore.sol::1167 => for (uint256 i = 0; i < 255; ++i) {
VoteEscrowDelegation.sol::171 => for (uint256 index = 0; index < delegated.length; index++) {
VoteEscrowDelegation.sol::189 => for (uint256 index = 0; index < delegatednft.length; index++) {
VoteEscrowDelegation.sol::199 => for (uint256 i; i < _array.length; i++) {

[G-12] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

use <x> = <x> + <y> or <x> = <x> - <y> instead to save gas

TokenUriHelper.sol::143 => digits -= 1;
VoteEscrowCore.sol::499 => ownerToNFTokenCount[_to] += 1;
VoteEscrowCore.sol::512 => ownerToNFTokenCount[_from] -= 1;
VoteEscrowCore.sol::748 => t_i += WEEK;
VoteEscrowCore.sol::755 => last_point.bias -= last_point.slope * int128(int256(t_i - last_checkpoint));
VoteEscrowCore.sol::756 => last_point.slope += d_slope;
VoteEscrowCore.sol::768 => _epoch += 1;
VoteEscrowCore.sol::784 => last_point.slope += (u_new.slope - u_old.slope);
VoteEscrowCore.sol::785 => last_point.bias += (u_new.bias - u_old.bias);
VoteEscrowCore.sol::803 => old_dslope += u_old.slope;
VoteEscrowCore.sol::805 => old_dslope -= u_new.slope; // It was a new deposit, not extension
VoteEscrowCore.sol::812 => new_dslope -= u_new.slope; // old slope disappeared at this point
VoteEscrowCore.sol::847 => _locked.amount += int128(int256(_value));
VoteEscrowCore.sol::1071 => last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts));
VoteEscrowCore.sol::1145 => block_time += (d_t * (_block - point_0.blk)) / d_block;
VoteEscrowCore.sol::1148 => upoint.bias -= upoint.slope * int128(int256(block_time - upoint.ts));
VoteEscrowCore.sol::1168 => t_i += WEEK;
VoteEscrowCore.sol::1175 => last_point.bias -= last_point.slope * int128(int256(t_i - last_point.ts));
VoteEscrowCore.sol::1179 => last_point.slope += d_slope;

[G-13] abi.encode() is less efficient than abi.encodePacked()

use abi.encodePacked() where possible to save gas

GolomTrader.sol::102 => abi.encode(
GolomTrader.sol::115 => abi.encode(
GolomTrader.sol::130 => abi.encode(
GolomTrader.sol::414 => bytes32 computedHash = keccak256(abi.encode(leaf));

[G-14] Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

VoteEscrowCore.sol::1011 => require(block.timestamp >= _locked.end, "The lock didn't expire");

[G-15] Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas. Consequences: each usage of a constant costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library)

RewardDistributor.sol::48 => uint256 constant dailyEmission = 600000 * 10**18;
RewardDistributor.sol::57 => uint256 constant secsInDay = 24 * 60 * 60;
VoteEscrowCore.sol::296 => uint256 internal constant MAXTIME = 4 * 365 * 86400;
VoteEscrowCore.sol::297 => int128 internal constant iMAXTIME = 4 * 365 * 86400;

[G-16] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) Saves 6 gas PER LOOP

GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
VoteEscrowDelegation.sol::199 => for (uint256 i; i < _array.length; i++) {

[G-17] Use bytes32 instead of string

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

VoteEscrowCore.sol::317 => string public constant name = 'veNFT';
VoteEscrowCore.sol::318 => string public constant symbol = 'veNFT';
VoteEscrowCore.sol::319 => string public constant version = '1.0.0';

[G-18] Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, multiple require statements with 1 condition per require statement should be used to save gas:

VoteEscrowCore.sol::538 => require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
VoteEscrowCore.sol::894 => require(attachments[_from] == 0 && !voted[_from], 'attached');
VoteEscrowCore.sol::1008 => require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
VoteEscrowDelegation.sol::239 => require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

[G-19] Usage of assert() instead of require()

Between solc 0.4.10 and 0.8.0, require() used REVERT (0xfd) opcode which refunded remaining gas on failure while assert() used INVALID (0xfe) opcode which consumed all the supplied gas. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking (properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix).

VoteEscrowCore.sol::493 => assert(idToOwner[_tokenId] == address(0));
VoteEscrowCore.sol::506 => assert(idToOwner[_tokenId] == _from);
VoteEscrowCore.sol::519 => assert(idToOwner[_tokenId] == _owner);
VoteEscrowCore.sol::666 => assert(_operator != msg.sender);
VoteEscrowCore.sol::679 => assert(_to != address(0));
VoteEscrowCore.sol::861 => assert(IERC20(token).transferFrom(from, address(this), _value));
VoteEscrowCore.sol::977 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::981 => assert(_value > 0); // dev: need non-zero value
VoteEscrowCore.sol::991 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::1007 => assert(_isApprovedOrOwner(msg.sender, _tokenId));
VoteEscrowCore.sol::1023 => assert(IERC20(token).transfer(msg.sender, value));
VoteEscrowCore.sol::1110 => assert(_block <= block.number);
VoteEscrowCore.sol::1206 => assert(_block <= block.number);