Golom contest - MiloTruck's results

• Low Issues
  • Use of assert() statements
  • Floating compiler versions
• Non-Critical Issues
  • Missing error message in require statements
  • event is missing indexed fields

Low Issues

Use of assert() statements

Contracts use assert() instead of require() in multiple places. As seen in Solidity's documentation:

Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.

Consider using require() statements instead of assert(), or use if statements with revert() instead.

Instances where assert() is used:

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); // dev: need non-zero value
 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);

Floating compiler versions

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

Findings

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

Recommended Mitigation Steps Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

Non-Critical Issues

Missing error message in require statements

It is recommended to have descriptive error messages for all require statements to provide users with information should a transaction fail.

Consider adding an error message to the following require statements:

contracts/vote-escrow/VoteEscrowDelegation.sol:
 245:        require(_isApprovedOrOwner(_sender, _tokenId));

contracts/vote-escrow/VoteEscrowCore.sol:
 360:        require(_entered_state == _not_entered);
 540:        require(_isApprovedOrOwner(_sender, _tokenId));
 646:        require(owner != address(0));
 648:        require(_approved != owner);
 652:        require(senderIsOwner || senderIsApprovedForAll);
 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);
 895:        require(_from != _to);
 896:        require(_isApprovedOrOwner(msg.sender, _from));
 897:        require(_isApprovedOrOwner(msg.sender, _to));
 927:        require(_value > 0); // dev: need non-zero value
 944:        require(_value > 0); // dev: need non-zero value

contracts/rewards/RewardDistributor.sol:
  88:        require(msg.sender == trader);
 144:        require(epochs[index] < epoch);
 158:        require(epochs[index] < epoch);

contracts/core/GolomTrader.sol:
 220:        require(msg.sender == o.reservedAddress);

 285:        require(
 286:            o.totalAmt * amount >
 287:                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
 288:        ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

 291:        require(msg.sender == o.reservedAddress);
 293:        require(o.orderType == 1);
 295:        require(status == 3);
 296:        require(amountRemaining >= amount);
 313:        require(o.signer == msg.sender);
 342:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
 345:        require(msg.sender == o.reservedAddress);
 347:        require(o.orderType == 2);
 349:        require(status == 3);
 350:        require(amountRemaining >= amount);

event is missing indexed fields

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

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

contracts/vote-escrow/VoteEscrowCore.sol:
  67:        event ApprovalForAll(address indexed owner, address indexed operator, bool approved);

 284:        event Deposit(
 285:            address indexed provider,
 286:            uint256 tokenId,
 287:            uint256 value,
 288:            uint256 indexed locktime,
 289:            DepositType deposit_type,
 290:            uint256 ts
 291:        );

 292:        event Withdraw(address indexed provider, uint256 tokenId, uint256 value, uint256 ts);

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

• Gas Report
  • For-loops: Index initialized with default value
  • For-Loops: Cache array length outside of loops
  • For-Loops: Index increments can be left unchecked
  • Arithmetics: ++i costs less gas compared to i++ or i += 1
  • Arithmetics: Use != 0 instead of > 0 for unsigned integers in require statements
  • Arithmetics: Use Shift Right/Left instead of Division/Multiplication if possible
  • Visibility: Consider declaring constants as non-public to save gas
  • Errors: Reduce the length of error messages (long revert strings)
  • Errors: Use multiple require statements instead of &&
  • Duplicated require() checks should be refactored to a modifier or function
  • Errors: Use custom errors instead of revert strings
  • Unnecessary initialization of variables with default values
  • Storage variables should be declared immutable when possible
  • Variables declared as constant are expressions, not constants
  • State variables should be cached in stack variables rather than re-reading them from storage
  • Remove or replace unused state variables
  • Using bool for storage incurs overhead
  • Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
  • internal functions only called once can be inlined to save gas
  • keccak256() should only need to be called on a specific string literal once
  • Multiple accesses of a mapping/array should use a local variable cache

Gas Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

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

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/vote-escrow/VoteEscrowDelegation.sol:
 171:        for (uint256 index = 0; index < delegated.length; index++) {
 189:        for (uint256 index = 0; index < delegatednft.length; 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/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/core/GolomTrader.sol:
 415:        for (uint256 i = 0; i < proof.length; i++) {

For-Loops: Cache array length outside of loops

Reading an array's length at each iteration has the following gas overheads:

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use mload (3 gas)
• calldata arrays use calldataload (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset. This would save around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

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/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++) {

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

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, index increments can be left unchecked to save 30-40 gas per loop iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

Consider making the following change to these lines:

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/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/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/core/GolomTrader.sol:
 415:        for (uint256 i = 0; i < proof.length; i++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/vote-escrow/TokenUriHelper.sol:
 138:        digits++;
 143:        digits -= 1;

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/VoteEscrowCore.sol:
 768:        _epoch += 1;

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/core/GolomTrader.sol:
 415:        for (uint256 i = 0; i < proof.length; i++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers in require statements

uint will never go below 0. Thus, checking if != 0 rather than > 0 is sufficient, which would save 6 gas per instance.

Consider changing > 0 to != 0 in these lines:

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

Arithmetics: 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 MUL and DIV opcodes use 5 gas, the SHL and SHR opcodes only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

For example, the following code:

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

can be changed to:

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Consider making this change to the following lines:

contracts/vote-escrow/TokenUriHelper.sol:
  17:        uint256 encodedLen = 4 * ((len + 2) / 3);

contracts/vote-escrow/VoteEscrowDelegation.sol:
 150:        uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

contracts/vote-escrow/VoteEscrowCore.sol:
 296:        uint256 internal constant MAXTIME = 4 * 365 * 86400;
 297:        int128 internal constant iMAXTIME = 4 * 365 * 86400;
1049:        uint256 _mid = (_min + _max + 1) / 2;
1120:        uint256 _mid = (_min + _max + 1) / 2;

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas. If needed, the value can be read from the verified contract source code.

Gas savings occur as the compiler does not have to create non-payable getter functions for deployment calldata and add another entry to the method ID table.

Consider changing the visibility of the following from public to internal or private:

contracts/vote-escrow/VoteEscrowCore.sol:
 319:        string public constant version = '1.0.0';

Errors: Reduce the length of error messages (long revert strings)

As seen here, revert strings longer than 32 bytes would incur an additional mstore (3 gas) for each extra 32-byte chunk. Furthermore, there are additional runtime costs for memory expansion and stack operations when the revert condition is met.

Thus, shortening revert strings to fit within 32 bytes would help to reduce runtime gas costs when the revert condition is met.

In these instances, consider shortening revert strings or using custom errors:

contracts/vote-escrow/VoteEscrowDelegation.sol:
  73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

contracts/vote-escrow/VoteEscrowCore.sol:
 929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
 945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
 983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

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

contracts/rewards/RewardDistributor.sol:
 181:        require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
 292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
 309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

Errors: Use multiple require statements instead of &&

Instead of using a single require statement with the && operator, use multiple require statements. With reference to this issue, this helps to reduce runtime gas cost at the expense of a larger deployment gas cost, which becomes cheaper with enough runtime calls.

A require statement can be split as such:

// Original code:
require(a && b, 'error');

// Changed to:
require(a, 'error: a');
require(b, 'error: b');

Instances where multiple require statements should be used:

contracts/vote-escrow/VoteEscrowDelegation.sol:
 239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

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');

Duplicated require() checks should be refactored to a modifier or function

If a require() statement that is used to validate function parameters or global variables is present across multiple functions in a contract, it should be rewritten into modifier if possible. This would help to reduce code deployment size, which saves gas, and improve code readability.

The following require() statements are repeated multiple times:

contracts/vote-escrow/VoteEscrowDelegation.sol:
  72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
 130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
 186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
 211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

contracts/vote-escrow/VoteEscrowCore.sol:
 538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
 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);
 927:        require(_value > 0); // dev: need non-zero value
 928:        require(_locked.amount > 0, 'No existing lock found');
 929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
 944:        require(_value > 0); // dev: need non-zero value
 946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
 982:        require(_locked.amount > 0, 'No existing lock found');
 983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
 999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

contracts/rewards/RewardDistributor.sol:
 144:        require(epochs[index] < epoch);
 158:        require(epochs[index] < epoch);
 173:        require(address(ve) != address(0), ' VE not added yet');
 220:        require(address(ve) != address(0), ' VE not added yet');

contracts/core/GolomTrader.sol:
 220:        require(msg.sender == o.reservedAddress);
 235:        require(amount == 1, 'only 1 erc721 at 1 time');
 291:        require(msg.sender == o.reservedAddress);
 295:        require(status == 3);
 296:        require(amountRemaining >= amount);
 299:        require(amount == 1, 'only 1 erc721 at 1 time');
 345:        require(msg.sender == o.reservedAddress);
 349:        require(status == 3);
 350:        require(amountRemaining >= amount);
 359:        require(amount == 1, 'only 1 erc721 at 1 time');

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors can be used rather than require statements.

Taken from Custom Errors in Solidity:

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 reduce runtime gas costs as they save about 50 gas each time they are hit by avoiding having to allocate and store the revert string. Furthermore, not definiting error strings also help to reduce deployment gas costs.

Instances where custom errors can be used instead:

contracts/vote-escrow/VoteEscrowDelegation.sol:
  72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
  73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
  99:        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
 130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
 186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
 211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
 239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
 245:        require(_isApprovedOrOwner(_sender, _tokenId));

contracts/vote-escrow/VoteEscrowCore.sol:
 360:        require(_entered_state == _not_entered);
 538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
 540:        require(_isApprovedOrOwner(_sender, _tokenId));
 646:        require(owner != address(0));
 648:        require(_approved != owner);
 652:        require(senderIsOwner || senderIsApprovedForAll);
 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);
 894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
 895:        require(_from != _to);
 896:        require(_isApprovedOrOwner(msg.sender, _from));
 897:        require(_isApprovedOrOwner(msg.sender, _to));
 927:        require(_value > 0); // dev: need non-zero value
 928:        require(_locked.amount > 0, 'No existing lock found');
 929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
 944:        require(_value > 0); // dev: need non-zero value
 945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
 946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
 982:        require(_locked.amount > 0, 'No existing lock found');
 983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
 996:        require(_locked.end > block.timestamp, 'Lock expired');
 997:        require(_locked.amount > 0, 'Nothing is locked');
 998:        require(unlock_time > _locked.end, 'Can only increase lock duration');
 999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
1011:        require(block.timestamp >= _locked.end, "The lock didn't expire");
1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

contracts/governance/GolomToken.sol:
  24:        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
  43:        require(!isAirdropMinted, 'already minted');
  51:        require(!isGenesisRewardMinted, 'already minted');
  69:        require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');

contracts/rewards/RewardDistributor.sol:
  88:        require(msg.sender == trader);
 144:        require(epochs[index] < epoch);
 158:        require(epochs[index] < epoch);
 173:        require(address(ve) != address(0), ' VE not added yet');
 181:        require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
 184:        require(epochs[index] < epoch, 'cant claim for future epochs');
 185:        require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
 220:        require(address(ve) != address(0), ' VE not added yet');
 292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
 309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

contracts/core/GolomTrader.sol:
 177:        require(signaturesigner == o.signer, 'invalid signature');

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

 217:        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
 220:        require(msg.sender == o.reservedAddress);
 222:        require(o.orderType == 0, 'invalid orderType');
 226:        require(status == 3, 'order not valid');
 227:        require(amountRemaining >= amount, 'order already filled');
 235:        require(amount == 1, 'only 1 erc721 at 1 time');

 285:        require(
 286:            o.totalAmt * amount >
 287:                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
 288:        ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

 291:        require(msg.sender == o.reservedAddress);
 293:        require(o.orderType == 1);
 295:        require(status == 3);
 296:        require(amountRemaining >= amount);
 299:        require(amount == 1, 'only 1 erc721 at 1 time');
 313:        require(o.signer == msg.sender);
 342:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
 345:        require(msg.sender == o.reservedAddress);
 347:        require(o.orderType == 2);
 349:        require(status == 3);
 350:        require(amountRemaining >= amount);
 359:        require(amount == 1, 'only 1 erc721 at 1 time');
 455:        require(distributorEnableDate <= block.timestamp, 'not allowed');

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

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;

contracts/vote-escrow/VoteEscrowCore.sol:
 735:        uint256 block_slope = 0; // dblock/dt
1042:        uint256 _min = 0;
1113:        uint256 _min = 0;
1133:        uint256 d_block = 0;
1134:        uint256 d_t = 0;
1211:        uint256 dt = 0;

contracts/rewards/RewardDistributor.sol:
  45:        uint256 public epoch = 0;
 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;

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

This helps to save gas as it avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

Consider declaring these variables as immutable:

contracts/rewards/RewardDistributor.sol:
  46:        uint256 public startTime; // timestamp at which the contracts need to be activated
  68:        ERC20 public rewardToken;
  69:        ERC20 public weth;

Variables declared as constant are expressions, not 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.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/vote-escrow/VoteEscrowCore.sol:
 296:        uint256 internal constant MAXTIME = 4 * 365 * 86400;
 297:        int128 internal constant iMAXTIME = 4 * 365 * 86400;

contracts/rewards/RewardDistributor.sol:
  48:        uint256 constant dailyEmission = 600000 * 10**18;
  57:        uint256 constant secsInDay = 24 * 60 * 60;

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.

State variables should be cached in stack variables rather than re-reading them from storage

If a state variable is read from storage multiple times in a function, it should be cached in a stack variable.

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

Instances of state variable being read multiple times:

contracts/governance/GolomToken.sol:
             // executeSetMinter(): pendingMinter
  67:        minter = pendingMinter;

             // executeSetMinter(): pendingMinter
  70:        minter = pendingMinter;

contracts/rewards/RewardDistributor.sol:
             // addFee(): epoch
 106:        if (block.timestamp > startTime + (epoch) * secsInDay) {

             // addFee(): ve
 112:        uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /

             // addFee(): ve
 114:        uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

             // addFee(): epoch
 117:        uint256 previousEpochFee = epochTotalFee[epoch];

             // addFee(): epoch
 118:        epoch = epoch + 1;

             // addFee(): epoch
 119:        rewardStaker[epoch] = stakerReward;

             // addFee(): epoch
 120:        rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;

             // addFee(): epoch
 121:        rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

             // addFee(): epoch
 123:        epochBeginTime[epoch] = block.number;

             // addFee(): epoch
 125:        if (epoch == 1){

             // addFee(): epoch
 132:        emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee);

             // addFee(): epoch
 134:        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;

             // addFee(): epoch
 134:        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;

             // addFee(): epoch
 135:        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;

             // addFee(): epoch
 135:        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;

             // addFee(): epoch
 136:        epochTotalFee[epoch] = epochTotalFee[epoch] + fee;

             // addFee(): epoch
 136:        epochTotalFee[epoch] = epochTotalFee[epoch] + fee;

             // stakerRewards(): epoch
 224:        uint256[] memory unclaimedepochs = new uint256[](epoch);

             // stakerRewards(): epoch
 226:        for (uint256 index = 0; index < epoch; index++) {

Remove or replace unused state variables

For initialized variables, this would save a storage slot and reduce gas costs as follows:

• If the variable is assigned a non-zero value, saves Gsset (20000 gas).
• If it's assigned a zero value, saves Gsreset (2900 gas).

For uninitialized variables, there is no gas savings, but they should be removed to prevent confusion.

If the state variable is overriding an interface's public function, mark the variable as constant or immutable so that it does not use a storage slot, and manually add a getter function.

Instances of unused state variables:

contracts/vote-escrow/VoteEscrowDelegation.sol:
  35:        mapping(uint256 => uint256[]) public delegatedTokenIds;

contracts/rewards/RewardDistributor.sol:
  63:        mapping(uint256 => uint256) public rewardLP; // reward minted each epoc for LP
  66:        mapping(uint256 => uint256) public claimedUpto; // epoch upto which tokenid has claimed

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

Using bool for storage incurs overhead

Declaring storage variables as bool is more expensive compared to uint256, as explained here:

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) rather than true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past.

Consider redefining the following state variables to use uint256 instead of bool:

contracts/vote-escrow/VoteEscrowCore.sol:
 314:        mapping(uint256 => bool) public voted;
 341:        mapping(address => mapping(address => bool)) internal ownerToOperators;
 344:        mapping(bytes4 => bool) internal supportedInterfaces;

contracts/governance/GolomToken.sol:
  20:        bool public isAirdropMinted;
  21:        bool public isGenesisRewardMinted;

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

As seen from here:

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.

However, this does not apply to storage values as using reduced-size types might be beneficial to pack multiple elements into a single storage slot. Thus, where appropriate, use uint256/int256 and downcast when needed.

Consider using uint256/int256 instead of bool for the following:

contracts/vote-escrow/VoteEscrowCore.sol:
 261:        int128 bias;
 262:        int128 slope; // # -dweight / dt
 271:        int128 amount;
 375:        function get_last_user_slope(uint256 _tokenId) external view returns (int128) {
 697:        int128 old_dslope = 0;
 698:        int128 new_dslope = 0;
 749:        int128 d_slope = 0;
1169:        int128 d_slope = 0;

contracts/core/GolomTrader.sol:
  62:        uint8 v;

internal functions only called once can be inlined to save gas

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

Consider inlining the following internal functions:

contracts/vote-escrow/VoteEscrowDelegation.sol:
 116:        function _getCurrentDelegated(uint256 tokenId) internal view returns (uint256[] memory) {
 129:        function _getPriorDelegated(uint256 nftId, uint256 blockNumber) internal view returns (uint256[] memory) {
 198:        function removeElement(uint256[] storage _array, uint256 _element) internal {

contracts/vote-escrow/VoteEscrowCore.sol:
 452:        function _addTokenToOwnerList(address _to, uint256 _tokenId) internal {
 462:        function _removeTokenFromOwnerList(address _from, uint256 _tokenId) internal {
 517:        function _clearApproval(address _owner, uint256 _tokenId) internal {
 571:        function _isContract(address account) internal view returns (bool) {
 677:        function _mint(address _to, uint256 _tokenId) internal returns (bool) {
1107:        function _balanceOfAtNFT(uint256 _tokenId, uint256 _block) internal view returns (uint256) {

keccak256() should only need to be called on a specific string literal once

The result of keccak256() should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once.

Instances of keccak256() that can be saved to an immutable variable:

contracts/core/GolomTrader.sol:
 116:        keccak256('payment(uint256 paymentAmt,address paymentAddress)'),

 131:        keccak256(
 132:            'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)'
 133:        ),

Multiple accesses of a mapping/array should use a local variable cache

If a mapping/array is read with the same key/index multiple times in a function, it should be cached in a stack variable.

Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory.

Instances of a mapping/array being read multiple times:

contracts/vote-escrow/VoteEscrowDelegation.sol:
             // _getPriorDelegated(): checkpoints[nftId]
 138:        if (checkpoints[nftId][nCheckpoints - 1].fromBlock <= blockNumber) {

             // _getPriorDelegated(): checkpoints[nftId]
 139:        return checkpoints[nftId][nCheckpoints - 1].delegatedTokenIds;

             // _getPriorDelegated(): checkpoints[nftId]
 143:        if (checkpoints[nftId][0].fromBlock > blockNumber) {

             // _getPriorDelegated(): checkpoints[nftId]
 151:        Checkpoint memory cp = checkpoints[nftId][center];

             // _getPriorDelegated(): checkpoints[nftId]
 160:        return checkpoints[nftId][lower].delegatedTokenIds;

contracts/vote-escrow/VoteEscrowCore.sol:
             // approve(): idToOwner[_tokenId]
 644:        address owner = idToOwner[_tokenId];

             // approve(): idToOwner[_tokenId]
 650:        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);

             // _balanceOfAtNFT(): user_point_history[_tokenId]
1121:        if (user_point_history[_tokenId][_mid].blk <= _block) {

             // _balanceOfAtNFT(): user_point_history[_tokenId]
1128:        Point memory upoint = user_point_history[_tokenId][_min];

contracts/rewards/RewardDistributor.sol:
             // addFee(): epochTotalFee[epoch]
 117:        uint256 previousEpochFee = epochTotalFee[epoch];

             // addFee(): epochTotalFee[epoch]
 136:        epochTotalFee[epoch] = epochTotalFee[epoch] + fee;

             // multiStakerClaim(): epochBeginTime[1]
 191:        ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[1])) /

             // multiStakerClaim(): epochBeginTime[1]
 192:        ve.totalSupplyAt(epochBeginTime[1]);

             // multiStakerClaim(): epochBeginTime[epochs[index]]
 197:        (rewardStaker[epochs[index]] * ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) /

             // multiStakerClaim(): epochBeginTime[epochs[index]]
 198:        ve.totalSupplyAt(epochBeginTime[epochs[index]]);

             // multiStakerClaim(): epochBeginTime[epochs[index]]
 202:        ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) /

             // multiStakerClaim(): epochBeginTime[epochs[index]]
 203:        ve.totalSupplyAt(epochBeginTime[epochs[index]]);

             // stakerRewards(): claimed[tokenid]
 227:        unclaimedepochs[index]=claimed[tokenid][index];

             // stakerRewards(): claimed[tokenid]
 228:        if (claimed[tokenid][index] == 0){

             // stakerRewards(): epochBeginTime[1]
 233:        ve.balanceOfAtNFT(tokenid, epochBeginTime[1])) /

             // stakerRewards(): epochBeginTime[1]
 234:        ve.totalSupplyAt(epochBeginTime[1]);

             // stakerRewards(): epochBeginTime[index]
 239:        (rewardStaker[index] * ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /

             // stakerRewards(): epochBeginTime[index]
 240:        ve.totalSupplyAt(epochBeginTime[index]);

             // stakerRewards(): epochBeginTime[index]
 244:        ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /

             // stakerRewards(): epochBeginTime[index]
 245:        ve.totalSupplyAt(epochBeginTime[index]);

contracts/core/GolomTrader.sol:
             // validateOrder(): filled[hashStruct]
 189:        if (filled[hashStruct] >= o.tokenAmt) {

             // validateOrder(): filled[hashStruct]
 193:        return (3, hashStruct, o.tokenAmt - filled[hashStruct]);