Golom contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L151-L156

Vulnerability details

ADDRESS.CALL{VALUE:X}() SHOULD BE USED INSTEAD OF PAYABLE.TRANSFER()

Impact

When operations use a wrapped native token, the withdraw is being handled with a payable.transfer() method.

When withdrawing and refund extra ETH, the GolomTrader contract uses Solidity’s transfer() function.

Using Solidity's transfer() function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-Effects-Interactions" pattern, which is used in _settleBalances() and fillAsk(). Another option is using OpenZeppelin Contract’s ReentrancyGuard contract. 

Proof of Concept

contracts/core/GolomTrader.sol

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

References:

The issues with transfer() are outlined here

For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised, reference.

Lines of code

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

Vulnerability details

transferFrom() might result in NFT trapped in receiver contract

Impact

For the ERC721 transfer, transferFrom() is used instead of safeTransferFrom(). If the NFT receiver is a contract but does not implement method to handle the NFT received, the NFT will be stuck forever, and the process is irreversible. Or when the receiver address is wrong by mistake, the NFT will also be lost. Both cases will result in user asset loss.

Proof of Concept

contracts/core/GolomTrader.sol

236:        ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

300-301:
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

360-361:
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, tokenId);

In the case of receiver contract, the onERC721Received() method can provide more reliable check for the transfer result.

The reentrancy issue can be handled by following the "Check-Effects-Interactions" pattern.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use safeTransferFrom() for ERC721, just like for ERC1155.

contracts/core/GolomTrader.sol

238:        ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');

EVENT IS MISSING INDEXED FIELDS

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

contracts/core/GolomTrader.sol
    event OrderFilled(
        address indexed maker,
        address indexed taker,
        uint256 indexed orderType,
        bytes32 orderHash,
        uint256 price
    );

contracts/rewards/RewardDistributor.sol
    event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee);


contracts/vote-escrow/VoteEscrowCore.sol
    event Withdraw(address indexed provider, uint256 tokenId, uint256 value, uint256 ts);

contracts/vote-escrow/VoteEscrowDelegation.sol
    event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

Typo in the comment

"epoc" should be "epoch" "echange" should be "echange": contracts/rewards/RewardDistributor.sol

61-67:
    mapping(uint256 => uint256) public rewardTrader; // reward minted each epoc for trader
    mapping(uint256 => uint256) public rewardExchange; // reward minted each epoc for exhange
    mapping(uint256 => uint256) public rewardLP; // reward minted each epoc for LP
    mapping(uint256 => uint256) public rewardStaker; // reward minted each epoc for stakers

contracts/rewards/RewardDistributor.sol

"begiining" should be "beginning".

111：
            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets 

"there" should be "their".

140:
    // allows sellers of nft to claim there previous epoch rewards

Unused/empty receive()/fallback() function

Sometimes Ether might be mistakenly sent to the contract. It's better to prevent this from happening.

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

contracts/rewards/RewardDistributor.sol contracts/core/GolomTrader.sol

    fallback() external payable {}

    receive() external payable {}

Unreachable if statement

contracts/core/GolomTrader.sol

177-180:
        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Due to the require() statement, signaturesigner has to be the same as o.signer, the if block won't be executed

Suggestion: Remove the require() statement or the if block.

duplicate nonreentrant() modifier

Modifier nonreentrant() is defined in: contracts/vote-escrow/VoteEscrowCore.sol

356-364:
    uint8 internal constant _not_entered = 1;
    uint8 internal constant _entered = 2;
    uint8 internal _entered_state = 1;
    modifier nonreentrant() {
        require(_entered_state == _not_entered);
        _entered_state = _entered;
        _;
        _entered_state = _not_entered;
    }

It seems a duplicate, because in contracts/core/GolomTrader.sol openzeppelin ReentrancyGuard is used. The implementations are similar.

Suggestion: Just use the OZ modifier just like GolomTrader.sol. Some deployment cost can be saved.

Unused import lib

contracts/rewards/RewardDistributor.sol

import 'hardhat/console.sol';

It's not used and can be removed.

Incomplete comment

contracts/core/GolomTrader.sol

160:    ///      OrderStatus = 1 , if deadline has been

No need for nonReentrant for ERC20

A nonReentrant modifier is used in contracts/vote-escrow/VoteEscrowCore.sol

    function deposit_for() external nonreentrant {}
    function create_lock_for() external nonreentrant returns () {}
    function create_lock() external nonreentrant returns () {
    function increase_amount() external nonreentrant {}
    function increase_unlock_time() external nonreentrant {}
    function withdraw() external nonreentrant {}

They eventually call the function IERC20(token).transfer() (some through _deposit_for()). However there won't be reentrency for ERC20, since no fallback()/receive() can be triggered, also no hook function like onReceive().

Suggestion: Remove the modifier and save some gas.

LARGE MULTIPLES OF TEN SHOULD USE SCIENTIFIC NOTATION (E.G. 1E6) RATHER THAN DECIMAL LITERALS (E.G. 1000000), FOR READABILITY

Constant variables can be used as this would make the code more maintainable and readable while costing nothing gas-wise.

contracts/rewards/RewardDistributor.sol

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

contracts/rewards/RewardDistributor.sol

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

contracts/vote-escrow/VoteEscrowCore.sol

308:    mapping(uint256 => Point[1000000000]) public 

EVENTS NOT EMITTED FOR IMPORTANT STATE CHANGES

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

contracts/governance/GolomToken.sol

    function setMinter(address _minter) external onlyOwner {
        pendingMinter = _minter;
        minterEnableDate = block.timestamp + 1 days;
    }

contracts/vote-escrow/VoteEscrowCore.sol

    function setVoter(address _voter) external {
        require(msg.sender == voter);
        voter = _voter;
    }

contracts/vote-escrow/VoteEscrowDelegation.sol

    function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {
        MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;
    }

Suggestion: Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity.

NATSPEC not complete

contracts/vote-escrow/VoteEscrowCore.sol 868-912:

Suggestion: Follow NATSPEC.

FOR-LOOPS GAS-SAVING

There are several for loops can be improved to save gas

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++) {
157:
        for (uint256 index = 0; index < epochs.length; index++) {
180:
        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
183:
        for (uint256 index = 0; index < epochs.length; index++) {
226:
        for (uint256 index = 0; index < epoch; index++) {
258:
        for (uint256 index = 0; index < epoch; index++) {
273:
        for (uint256 index = 0; index < epoch; index++) {

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

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

contracts/vote-escrow/TokenUriHelper.sol
28-30:
        for {
            let i := 0
        } lt(i, len) {

1. LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables, the comparison can been seen in the reference link at the end (the length() function for test).

    uint length = names.length;

2. ++I COSTS LESS GAS COMPARED TO I++ OR I += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked.

3. NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Summary: Take the following as one example

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

can be written as

    uint length = names.length;
    unchecked {
        for (uint256 i; i < length; ++i) {
            treeLength += IModule(_modules[i]).getLeafNodes().length;
        }
    }

And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

Use bit shift instead of power operation

encode() seem to be frequently called functions.

contracts/vote-escrow/TokenUriHelper.sol

        uint256 encodedLen = 4 * ((len + 2) / 3);

Suggestion: Change the above to

        uint256 encodedLen = ((len + 2) / 3) >> 2;

The demo of the gas comparison can be seen here.

REDUCE THE SIZE OF ERROR MESSAGES

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

contracts/governance/GolomToken.sol

24:
        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) reference

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

The following files have multiple require() statements can use custom errors: contracts/core/GolomTrader.sol contracts/governance/GolomToken.sol contracts/rewards/RewardDistributor.sol contracts/vote-escrow/VoteEscrowCore.sol contracts/vote-escrow/VoteEscrowDelegation.sol

SPLITTING REQUIRE() STATEMENTS THAT USE &&

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

contracts/vote-escrow/VoteEscrowCore.sol

538:
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

894:
        require(attachments[_from] == 0 && !voted[_from], 'attached');
1008:
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

contracts/vote-escrow/VoteEscrowDelegation.sol

239:
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

The demo of the gas comparison can be seen here.

FUNCTIONS ONLY CALLED OUTSIDE OF THE CONTRACT CAN BE DECLARED AS EXTERNAL

External call cost is less expensive than of public functions.

contracts/core/GolomTrader.sol

    function fillAsk() public payable nonReentrant {}

    function fillBid() public payable nonReentrant {}
    function cancelOrder(Order calldata o) public nonReentrant {}
    function fillCriteriaBid() public nonReentrant {}

contracts/rewards/RewardDistributor.sol

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {}
    function traderClaim(address addr, uint256[] memory epochs) public {}
    function exchangeClaim(address addr, uint256[] memory epochs) public {}
    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {}

And the arguments can use calldata instead of memory to further reduce gas usage, because the arguments are read-only.

The demo of the gas comparison for calldata and memory can be seen here.

startTime can be set as constant

startTime is not changed elsewhere. It can be constant, rather than state variable.

contracts/rewards/RewardDistributor.sol

84:     startTime = 1659211200;

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot

contracts/rewards/RewardDistributor.sol

// key epoch
60-67:
    mapping(uint256 => uint256) public epochTotalFee; // total fee of epoch
    mapping(uint256 => uint256) public rewardTrader; // reward minted each epoc for trader
    mapping(uint256 => uint256) public rewardExchange; // reward minted each epoc for exhange
    mapping(uint256 => uint256) public rewardLP; // reward minted each epoc for LP
    mapping(uint256 => uint256) public rewardStaker; // reward minted each epoc for stakers
    mapping(uint256 => uint256) public epochBeginTime; // what time previous epoch ended
    mapping(uint256 => uint256) public claimedUpto; // epoch upto which tokenid has claimed
    mapping(uint256 => mapping(uint256 => uint256)) public claimed; // epoch upto which tokenid has claimed

contracts/vote-escrow/VoteEscrowCore.sol

// key user
308-310:
    mapping(uint256 => Point[1000000000]) public user_point_history; // user -> Point[user_epoch]

    mapping(uint256 => uint256) public user_point_epoch;

// key NFT ID
325-329, 37-338:
    /// @dev Mapping from NFT ID to the address that owns it.
    mapping(uint256 => address) internal idToOwner;

    /// @dev Mapping from NFT ID to approved address.
    mapping(uint256 => address) internal idToApprovals;

    /// @dev Mapping from NFT ID to index of owner
    mapping(uint256 => uint256) internal tokenToOwnerIndex;


// key owner address
331-335, 340-341:
    /// @dev Mapping from owner address to count of his tokens.
    mapping(address => uint256) internal ownerToNFTokenCount;

    /// @dev Mapping from owner address to mapping of index to tokenIds
    mapping(address => mapping(uint256 => uint256)) internal ownerToNFTokenIdList;

    /// @dev Mapping from owner address to mapping of operator addresses.
    mapping(address => mapping(address => bool)) internal ownerToOperators;

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata.

The demo of the gas comparison can be seen here.

Variable re-arrangement by storage packing

In struct Order, bool isERC721 can be placed next to uint8 v, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

contracts/core/GolomTrader.sol

    struct Order {
        bool isERC721;
        uint8 v;
    }

Reference: Layout of State Variables in Storage.

modifier only used once

contracts/rewards/RewardDistributor.sol: modifier onlyTrader() is only used in function addFee().

contracts/governance/GolomToken.sol modifier onlyMinter() is only used in function mint().

Suggestion: Consider add a require() statement directly inside the function. Can save some additional complier overhead.

Unreachable if statement

contracts/core/GolomTrader.sol

177-180:
        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Due to the require() statement, signaturesigner has to be the same as o.signer, the if block won't be executed

Suggestion: Remove the require() statement or the if block.

duplicate nonreentrant() modifier

Modifier nonreentrant() is defined in: contracts/vote-escrow/VoteEscrowCore.sol

356-364:
    uint8 internal constant _not_entered = 1;
    uint8 internal constant _entered = 2;
    uint8 internal _entered_state = 1;
    modifier nonreentrant() {
        require(_entered_state == _not_entered);
        _entered_state = _entered;
        _;
        _entered_state = _not_entered;
    }

It seems a duplicate, because in contracts/core/GolomTrader.sol openzeppelin ReentrancyGuard is used. The implementations are similar.

Suggestion: Just use the OZ modifier just like GolomTrader.sol. Some deployment cost can be saved.

Used require() instead of assert()

require() is normally used for checking error conditions on inputs and return values, while assert() for invariant checking. Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. require() will refund any remaining gas to the caller, but assert() won't refund. From a gas point of view, require() is more user friendly.

There are multiple assert() can be replaced by require().

Further reference SWC-110 and SWC-123.

contracts/vote-escrow/VoteEscrowCore.sol

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

DUPLICATED REQUIRE()/ASSERT() CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTION

contracts/vote-escrow/VoteEscrowCore.sol

1110:        assert(_block <= block.number);
1206:        assert(_block <= block.number);

contracts/rewards/RewardDistributor.sol

144:            require(epochs[index] < epoch);
158:            require(epochs[index] < epoch);
184:            require(epochs[index] < epoch, 'cant claim for future epochs');

contracts/vote-escrow/VoteEscrowCore.sol

869:        require(msg.sender == voter);
874:        require(msg.sender == voter);
879:        require(msg.sender == voter);
884:        require(msg.sender == voter);
889:        require(msg.sender == voter);

928:        require(_locked.amount > 0, 'No existing lock found');
997:        require(_locked.amount > 0, 'Nothing is locked');

929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
996:        require(_locked.end > block.timestamp, 'Lock expired');

contracts/vote-escrow/VoteEscrowDelegation.sol

130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

contracts/rewards/RewardDistributor.sol

45:         contracts/rewards/RewardDistributor.sol
142:        uint256 reward = 0;
156:        uint256 reward = 0;
175:        uint256 reward = 0;
176:        uint256 rewardEth = 0;
222:        uint256 reward = 0;
223:        uint256 rewardEth = 0;
257:        uint256 reward = 0;
272:        uint256 reward = 0;

contracts/vote-escrow/VoteEscrowCore.sol

697:         int128 old_dslope = 0;
698:         int128 new_dslope = 0;
735:         uint256 block_slope = 0;
749:         int128 d_slope = 0;
1042:        uint256 _min = 0;
1113:        uint256 _min = 0;
1133:        uint256 d_block = 0;
1134:        uint256 d_t = 0;
1169:        int128 d_slope = 0;
1121:        uint256 dt = 0;

contracts/vote-escrow/VoteEscrowDelegation.sol

50:         uint256 public MIN_VOTING_POWER_REQUIRED = 0;
147:        uint256 lower = 0;
170:        uint256 votes = 0;
188:        uint256 votes = 0;

Obsolete/filled/cancelled/closed orders can be deleted to refund gas

There is a deadline field in the order struct, and the deadline is checked for order validity. Also, when an order is completely filled or cancelled, the filled[] mapping is marked.

However no deletion mechanism is found to recycle the storage space. When an order's deadline has passed or the order has been filled/cancelled, the storage can be deleted for gas refund.

        delete filled[hashStruct];

Variables referred multiple times can be cached in local memory

epochBeginTime[epochs[index]] can be cached. contracts/rewards/RewardDistributor.sol

195-203:
        reward =
        reward +
        (rewardStaker[epochs[index]] * ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) /
        ve.totalSupplyAt(epochBeginTime[epochs[index]]);
        rewardEth =
        rewardEth +
        (epochTotalFee[epochs[index]] *
                ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) /
        ve.totalSupplyAt(epochBeginTime[epochs[index]]);

epochBeginTime[index] can be cached.

239-245:
        (rewardStaker[index] * ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /
        ve.totalSupplyAt(epochBeginTime[index]);
        rewardEth =
        rewardEth +
        (epochTotalFee[index] *
                ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /
        ve.totalSupplyAt(epochBeginTime[index]);

Variables declaration can be moved outside the loop, since each declaration has overhead. If needed, these variables can be set to 0 after each use.

contracts/vote-escrow/VoteEscrowCore.sol

int128 d_slope

745-775:
        for (uint256 i = 0; i < 255; ++i) {
            int128 d_slope = 0;
            if (t_i > t) {
                t_i = t;
            } else {
                d_slope = slope_changes[t_i];
            }
            // ...

            last_point.slope += d_slope;
        }

uint256 _mid

1044-1055:
        for (uint256 i = 0; i < 128; ++i) {
                // ...
                uint256 _mid = (_min + _max + 1) / 2;
                // ...
        }

uint256 _mid

1115-1126:
        for (uint256 i = 0; i < 128; ++i) {
                // ...
                uint256 _mid = (_min + _max + 1) / 2;
                // ...
        }

int128 d_slope

1167-1181:
        for (uint256 i = 0; i < 255; ++i) {
            int128 d_slope = 0;
            if (t_i > t) {
                t_i = t;
            } else {
                d_slope = slope_changes[t_i];
            }
            // ...

            last_point.slope += d_slope;
        }

The demo of the gas comparison can be seen here.

No need for nonReentrant for ERC20

A nonReentrant modifier is used in contracts/vote-escrow/VoteEscrowCore.sol

    function deposit_for() external nonreentrant {}
    function create_lock_for() external nonreentrant returns () {}
    function create_lock() external nonreentrant returns () {
    function increase_amount() external nonreentrant {}
    function increase_unlock_time() external nonreentrant {}
    function withdraw() external nonreentrant {}

They eventually call the function IERC20(token).transfer() (some through _deposit_for()). However there won't be reentrency for ERC20, since no fallback()/receive() can be triggered, also no hook function like onReceive().

Suggestion: Remove the modifier and save some gas.

> 0 uses less gas than != 0

!= 0 costs less gas compared to > 0 for unsigned integers with the optimizer enabled (6 gas). Here is the code as a demo. Opcode discussion.

Especially for some frequently used functions like payEther().

contracts/core/GolomTrader.sol

151-156:
    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

250:        
        if (o.refererrAmt > 0 && referrer != address(0)) {
387:
        if (o.refererrAmt > 0 && referrer != address(0)) {

contracts/rewards/RewardDistributor.sol

124:
        if (previousEpochFee > 0) {

contracts/vote-escrow/VoteEscrowCore.sol

727:
        if (_epoch > 0) {
927-928:
        require(_value > 0); 
        require(_locked.amount > 0, 'No existing lock found');
944:
        require(_value > 0); 
981-982:
        assert(_value > 0); 
        require(_locked.amount > 0, 'No existing lock found');
997:
        require(_locked.amount > 0, 'Nothing is locked');

contracts/vote-escrow/VoteEscrowDelegation.sol

78:
        if (nCheckpoints > 0) {
103:
        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {

Suggestion:

• Changing > 0 with != 0.
• Enable the Optimizer.

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.

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.

contracts/rewards/RewardDistributor.sol

98:     function addFee(address[2] memory addr, uint256 fee) public onlyTrader {}
285:    function changeTrader(address _trader) external onlyOwner {
291:    function executeChangeTrader() external onlyOwner {
298:    function addVoteEscrow(address _voteEscrow) external onlyOwner {
308:    function executeAddVoteEscrow() external onlyOwner {

contracts/core/GolomTrader.sol

444:    function setDistributor(address _distributor) external onlyOwner {
454:    function executeSetDistributor() external onlyOwner {

contracts/governance/GolomToken.sol

36:     function mint(address _account, uint256 _amount) external onlyMinter {
42:     function mintAirdrop(address _airdrop) external onlyOwner {
50:    function mintGenesisReward(address _rewardDistributor) external onlyOwner {
58:    function setMinter(address _minter) external onlyOwner {
65:    function executeSetMinter() external onlyOwner {

contracts/vote-escrow/VoteEscrowDelegation.sol

260:    function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {

X = X + Y IS CHEAPER THAN X += Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y to save gas

contracts/vote-escrow/VoteEscrowCore.sol

499:     ownerToNFTokenCount[_to] += 1;
748:     t_i += WEEK;
756:     last_point.slope += d_slope;
768:     _epoch += 1;
784:     last_point.slope += (u_new.slope - u_old.slope);
785:     last_point.bias += (u_new.bias - u_old.bias);
803:     old_dslope += u_old.slope;
847:     _locked.amount += int128(int256(_value));
1145:    block_time += (d_t * (_block - point_0.blk)) / d_block;
1168:    t_i += WEEK;
1179:    last_point.slope += d_slope;

512:     ownerToNFTokenCount[_from] -= 1;
755:     last_point.bias -= last_point.slope * int128(int256(t_i - last_checkpoint));
805:     old_dslope -= u_new.slope;
812:     new_dslope -= u_new.slope;
1071:    last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts));
1148:    upoint.bias -= upoint.slope * int128(int256(block_time - upoint.ts));
1175:    last_point.bias -= last_point.slope * int128(int256(t_i - last_point.ts));

MAKING SOME VARIABLE AS NON-PUBLIC

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.

The followings can be changed from public to private:

contracts/vote-escrow/VoteEscrowCore.sol

311:    mapping(uint256 => int128) public slope_changes;

INTERNAL FUNCTIONS ONLY CALLED ONCE CAN BE INLINED

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

contracts/vote-escrow/VoteEscrowCore.sol

    function _isContract(address account) internal view returns (bool) {
        // This method relies on extcodesize, which returns 0 for contracts in
        // construction, since the code is only stored at the end of the
        // constructor execution.
        uint256 size;
        assembly {
            size := extcodesize(account)
        }
        return size > 0;
    }

Function _isContract() is only called once in safeTransferFrom().

STATE VARIABLES ONLY SET IN THE CONSTRUCTOR SHOULD BE DECLARED IMMUTABLE

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

contracts/vote-escrow/VoteEscrowCore.sol

300:    address public token;