Golom contest - c3phas's results

Lines of code

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

Vulnerability details

Impact

When withdrawing ETH deposits, the payEther function in GolomTrader.sol contract uses Solidity’s transfer function. This has some notable shortcomings which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

1. The withdrawer smart contract does not implement a payable fallback function.
2. The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
3. 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.

Proof of Concept

File: GolomTrader.sol line 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
        }
    }

Tools Used

Visual Studio code

Recommended Mitigation Steps

Use call or the sendValue function available in OpenZeppelin Contract’s Address library which can be used to transfer Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

QA

Missing checks for address(0x0) when assigning values to address state variables

File: RewardDistributor.sol line 81

        trader = _trader;

File: VoteEscrowDelegation.sol line 53

        token = token;

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning which make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File:RewardDistributor.sol line 100 1000000000 * 10**18

        if (rewardToken.totalSupply() > 1000000000 * 10**18) {

File:RewardDistributor.sol 33,67,100 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L120-L121 File: GolomToken.sol 150_000_000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L44 File: GolomToken.sol 62_500_000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L52 File: GolomTrader.sol 50 and 10000
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L211-L214 File: GolomTrader.sol 50 and 10000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 File: GolomTrader.sol 50 and 10000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L254-L255 File: GolomTrader.sol 50 and 10000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L263 File: GolomTrader.sol 50 and 10000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L269 File: GolomTrader.sol 50 and 10000 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L381 File: VoteEscrowDelegation.sol
500 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L99

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File: GolomToken.sol line 2

pragma solidity ^0.8.11;

Typos

File: RewardDistributor.sol line 95

    /// @dev Add fees contributed by the Seller of nft and the exchange/frontend that facilated the trade

facilated instead of facilitated

File: RewardDistributor.sol line 101

            // if supply is greater then a billion dont mint anything, dont add trades

then instead of than

File: RewardDistributor.sol line 111

            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining

begiining instead of begining

File: RewardDistributor.sol line 154

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

there instead of their

File: RewardDistributor.sol line 154

    // allows exchange that facilated the nft trades to claim there previous epoch rewards

facilated instead of facilitated there instead of their

File: VoteEscrowDelegation.sol line 227

    /// @dev Exeute transfer of a NFT.

Exeute instead of Execute

File: VoteEscrowCore.sol line 526

    /// @dev Exeute transfer of a NFT.

Exeute instead of Execute

Missing revert strings

File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L220 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L285-L288 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L291 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L293 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L295 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L296 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L313 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L342 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L345 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L347 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L349 File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L350 File:RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L88 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L144 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L245 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L540

Natspec is incomplete

File: RewardDistributor.sol line 267-271

    /// @dev returns unclaimed golom rewards of a trader
    /// @param addr the nft id to claim rewards for all ids in the list must belong to 1 address
    function exchangeRewards(address addr) public view returns (
            uint256
        ){

Missing @return

Large multiples of ten should use scientific notation eg 1e9 rather than decimal literals eg 1000000000 for Readability

File: RewardDistributor.sol line 100

        if (rewardToken.totalSupply() > 1000000000 * 10**18) {

1000000000 should be modified to 1e9 10**18 can be written as 1e18

Remove Commented code

File: VoteEscrowDelegation.sol line 218-225

    // /// @notice Remove delegation by user
    // function removeDelegationByOwner(uint256 delegatedTokenId, uint256 ownerTokenId) external {
    //     require(ownerOf(ownerTokenId) == msg.sender, 'VEDelegation: Not allowed');
    //     uint256 nCheckpoints = numCheckpoints[delegatedTokenId];
    //     Checkpoint storage checkpoint = checkpoints[delegatedTokenId][nCheckpoints - 1];
    //     removeElement(checkpoint.delegatedTokenIds, delegatedTokenId);
    //     _writeCheckpoint(ownerTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    // }

Public functions that are never called from within the contracts should be declared external

Contracts can override their parents' functions and change the visibility from external to public

File: RewardDistributor.sol line 98

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {

File: RewardDistributor.sol line 141

    function traderClaim(address addr, uint256[] memory epochs) public {

File: RewardDistributor.sol line 155

    function exchangeClaim(address addr, uint256[] memory epochs) public {

File: RewardDistributor.sol line 172

    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {

File: RewardDistributor.sol line 215

    function stakerRewards(uint256 tokenid) public view returns (

File: RewardDistributor.sol line 254

    function traderRewards(address addr) public view returns (

File: RewardDistributor.sol line 269

    function exchangeRewards(address addr) public view returns (

File: VoteEscrowDelegation.sol line 185

    function getPriorVotes(uint256 tokenId, uint256 blockNumber) public view returns (uint256) {

FINDINGS

Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it

To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

File: VoteEscrowCore.sol line 465

VoteEscrowCore.sol._removeTokenFromOwnerList() : should declare tokenToOwnerIndex storage tokenToOwner = tokenToOwnerIndex[\_tokenId]

line 465

          uint256 current_index = tokenToOwnerIndex[_tokenId];//@audit - use the cached reference

line 471

          tokenToOwnerIndex[_tokenId] = 0;//@audit - use the cached reference

line 485

          tokenToOwnerIndex[_tokenId] = 0;//@audit - use the cached reference

File: VoteEscrowCore.sol line 493&495

VoteEscrowCore.sol._addTokenTo() : should declare idToOwner storage _idToOwner = idToOwner[\_tokenId] then use _idToOwner

    function _addTokenTo(address _to, uint256 _tokenId) internal {
        // Throws if `_tokenId` is owned by someone
       assert(idToOwner[_tokenId] == address(0));//@audit - use the cached reference for idToOwner[_tokenId]
        // Change the owner
        idToOwner[_tokenId] = _to;//@audit - use the cached reference for idToOwner[_tokenId]

File: VoteEscrowCore.sol line 506&508

VoteEscrowCore.sol._removeTokenFrom() : should declare idToOwner storage _idToOwner = idToOwner[_tokenId] then use _idToOwner(see audit tags for where to change)

    function _removeTokenFrom(address _from, uint256 _tokenId) internal {
        // Throws if `_from` is not the current owner
        assert(idToOwner[_tokenId] == _from);//@audit - use the cached reference for idToOwner[_tokenId]
        // Change the owner
        idToOwner[_tokenId] = address(0);//@audit - use the cached reference for idToOwner[_tokenId]

File: VoteEscrowCore.sol line 520&522

VoteEscrowCore.sol._clearApproval() : should declare idToApprovals storage _idToApprovals = idToApprovals[_tokenId] then use _idToApprovals(see audit tags for where to change)

if (idToApprovals[_tokenId] != address(0)) { //@audit - use the cached reference for idToApprovals[_tokenId]
   // Reset approvals
    idToApprovals[_tokenId] = address(0);//@audit - use the cached reference for idToApprovals[_tokenId]

No need to initialize state variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File: VoteEscrowDelegation.sol line 50

    uint256 public MIN_VOTING_POWER_REQUIRED = 0;

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource

File: VoteEscrowDelegation.sol line 79

            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];

The arithmetic operation nCheckpoints - 1 cannot underflow due to the check on line 78 which ensures that nCheckpoints is greater than 0 before performing the operation

File: VoteEscrowDelegation.sol line 119

        return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray;

The arithmetic operation nCheckpoints - 1 cannot underflow due to the check which ensures that nCheckpoints is greater than 0 before performing the operation.

File: VoteEscrowDelegation.sol line 138-140

        if (checkpoints[nftId][nCheckpoints - 1].fromBlock <= blockNumber) {
            return checkpoints[nftId][nCheckpoints - 1].delegatedTokenIds;
        }

The arithmetic operation nCheckpoints - 1 cannot underflow due to the check on line 133 which ensures that nCheckpoints is greater than 0 before performing the operation.

File: VoteEscrowDelegation.sol line 148

        uint256 upper = nCheckpoints - 1;

The arithmetic operation nCheckpoints - 1 cannot underflow due to the check on line 133 which ensures that nCheckpoints is greater than 0 before performing the operation.

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File: GolomTrader.sol line 409-428

        for (uint256 i = 0; i < proof.length;) {
            bytes32 proofElement = proof[i];
            if (computedHash <= proofElement) {
                // Hash(current computed hash + current element of the proof)
                computedHash = _efficientHash(computedHash, proofElement);
            } else {
                // Hash(current element of the proof + current computed hash)
                computedHash = _efficientHash(proofElement, computedHash);
            }

        }
        if (computedHash != root) {
            revert('invalid proof');
        }
    }

The above should be modified to:

        for (uint256 i = 0; i < proof.length; i++) {
            bytes32 proofElement = proof[i];
            if (computedHash <= proofElement) {
                // Hash(current computed hash + current element of the proof)
                computedHash = _efficientHash(computedHash, proofElement);
            } else {
                // Hash(current element of the proof + current computed hash)
                computedHash = _efficientHash(proofElement, computedHash);
            }
				   unchecked {
					     ++i;
					   }
        }
        if (computedHash != root) {
            revert('invalid proof');
        }
    }

Other Instances to modify File: RewardDistributor.sol line 140-152

    // allows sellers of nft to claim there previous epoch rewards
    function traderClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardTrader[epochs[index]] * feesTrader[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesTrader[addr][epochs[index]] = 0;
        }
        rewardToken.transfer(addr, reward);
    }

Index++ should be unchecked

File: RewardDistributor.sol line 155-166

    function exchangeClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesExchange[addr][epochs[index]] = 0;
        }
        rewardToken.transfer(addr, reward);
    }

Index++ should be unchecked

File: RewardDistributor.sol line 180

        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

tindex++ should be unchecked

File: RewardDistributor.sol line 183

            for (uint256 index = 0; index < epochs.length; index++) {

Index++ should be unchecked

File: VoteEscrowDelegation.sol line 171

        for (uint256 index = 0; index < delegated.length; index++) {

Index++ should be unchecked

File: VoteEscrowDelegation.sol line 189

        for (uint256 index = 0; index < delegatednft.length; index++) {

Index++ should be unchecked

File: VoteEscrowDelegation.sol line 199

    function removeElement(uint256[] storage _array, uint256 _element) internal {
        for (uint256 i; i < _array.length; i++) {

i++ should be unchecked

see resource

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3gas) Storage value should get cached in memory

NB: Some functions have been truncated where necessary to just show affected parts of the code

File: VoteEscrowCore.sol line 1166-1168

VoteEscrowCore.sol._supply_at(): WEEK should be cached

    function _supply_at(Point memory point, uint256 t) internal view returns (uint256) {
        Point memory last_point = point;
        uint256 t_i = (last_point.ts / WEEK) * WEEK;
        for (uint256 i = 0; i < 255; ++i) {
            t_i += WEEK;

File:VoteEscrowCore.sol Line 994

VoteEscrowCore.sol.increase_unlock_time(): WEEK should be cached

        uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; // Locktime is rounded down to weeks

File:VoteEscrowCore.sol line 942

VoteEscrowCore.sol.increase_unlock_time(): WEEK should be cached

        uint256 unlock_time = ((block.timestamp + _lock_duration) / WEEK) * WEEK; // Locktime is rounded down to weeks

File:VoteEscrowCore.sol line 705&709

VoteEscrowCore.sol._checkpoint(): iMAXTIME should be cached

            if (old_locked.end > block.timestamp && old_locked.amount > 0) {
                u_old.slope = old_locked.amount / iMAXTIME;
                u_old.bias = u_old.slope * int128(int256(old_locked.end - block.timestamp));
            }
            if (new_locked.end > block.timestamp && new_locked.amount > 0) {
                u_new.slope = new_locked.amount / iMAXTIME;
                u_new.bias = u_new.slope * int128(int256(new_locked.end - block.timestamp));
            }

SLOAD 1: line 705 SLOAD 2: line 709

File:VoteEscrowCore.sol line 744&748

VoteEscrowCore.sol._checkpoint(): WEEK should be cached

            uint256 t_i = (last_checkpoint / WEEK) * WEEK;
            for (uint256 i = 0; i < 255; ++i) {
                // Hopefully it won't happen that this won't get used in 5 years!
                // If it does, users will be able to withdraw but vote weight will be broken
                t_i += WEEK;

File: VoteEscrowCore.sol line 644 & 650

VoteEscrowCore.sol.approve(): idToOwner[_tokenId] should be cached(see audit tags)

    function approve(address _approved, uint256 _tokenId) public {
        address owner = idToOwner[_tokenId]; //@audit SLOAD 1 - cache idToOwner[_tokenId]
        // Throws if `_tokenId` is not a valid NFT
        require(owner != address(0));
        // Throws if `_approved` is the current owner
        require(_approved != owner);
        // Check requirements
        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);//@audit SLOAD 2
        bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
        require(senderIsOwner || senderIsApprovedForAll);
        // Set the approval
        idToApprovals[_tokenId] = _approved;

File: RewardDistributor.sol line 100,112,113,114

RewardDistributor.sol.addFee(): rewardToken.totalSupply() should be cached(The value is being read 4 times in this function)

    Line(100)    if (rewardToken.totalSupply() > 1000000000 * 10**18) { //@audit SLOAD 1
		
		Line(112) uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply()-//@audit SLOAD 2 rewardToken.balanceOf(address(ve)))) /
		Line(113) rewardToken.totalSupply(); //@audit SLOAD 3
		Line(114) uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();//@audit SLOAD 4

There are 4 SLOADS in the above function for rewardToken.totalSupply(). We can minimize these number by caching it in memory

Costly operations inside a loop

File: RewardDistributor.sol line 184

RewardDistributor.sol.multiStakerClaim(): epoch should be cached(see audit tags )

    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
            for (uint256 index = 0; index < epochs.length; index++) {
                require(epochs[index] < epoch, 'cant claim for future epochs');//@audit - epoch should be cached

Since epoch is a storage variable, we should cache it in memory outside the for loop and use the memory variable inside the for loop

File: RewardDistributor.sol line 158

RewardDistributor.sol.exchangeClaim(): epoch should be cached(see audit tags )

    function exchangeClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);//@audit - epoch should be cached

Since epoch is a storage variable, we should cache it in memory outside the for loop and use the memory variable inside the for loop

File: RewardDistributor.sol line 144

RewardDistributor.sol.traderClaim(): epoch should be cached(see audit tags )

    function traderClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch); //@audit - epoch should be cached

Since epoch is a storage variable, we should cache it in memory outside the for loop and use the memory variable inside the for loop

Cache the length of arrays in loops (saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: GolomTrader.sol line 409-428

    function _verifyProof(
       uint256 leaf,
       bytes32 root,
       bytes32[] memory proof
   ) public pure {
       bytes32 computedHash = keccak256(abi.encode(leaf));
       for (uint256 i = 0; i < proof.length; i++) {
           bytes32 proofElement = proof[i];
           if (computedHash <= proofElement) {

The above should be modified to

    function _verifyProof(
       uint256 leaf,
       bytes32 root,
       bytes32[] memory proof
   ) public pure {
       bytes32 computedHash = keccak256(abi.encode(leaf));
   uint256 length = proof.length;
       for (uint256 i = 0; i < length; i++) {
           bytes32 proofElement = proof[i];
           if (computedHash <= proofElement) {

Other instances to modify File: RewardDistributor.sol line 140-152

    // allows sellers of nft to claim there previous epoch rewards
    function traderClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardTrader[epochs[index]] * feesTrader[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesTrader[addr][epochs[index]] = 0;
        }
        rewardToken.transfer(addr, reward);
    }

epochs.length should be cached

File: RewardDistributor.sol line 155-166

    function exchangeClaim(address addr, uint256[] memory epochs) public {
        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesExchange[addr][epochs[index]] = 0;
        }
        rewardToken.transfer(addr, reward);
    }

epochs.length should be cached

File: RewardDistributor.sol line 180

        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

tokenids.length should be cached

File: RewardDistributor.sol line 183

            for (uint256 index = 0; index < epochs.length; index++) {

epochs.length should be cached

File: VoteEscrowDelegation.sol line 171

        for (uint256 index = 0; index < delegated.length; index++) {

delegated.length should be cached

File: VoteEscrowDelegation.sol line 189

        for (uint256 index = 0; index < delegatednft.length; index++) {

delegatednft.length should be cached

File: VoteEscrowDelegation.sol line 199

    function removeElement(uint256[] storage _array, uint256 _element) internal {
        for (uint256 i; i < _array.length; i++) {

_array.length should be cached especially because _array is a storage array

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, 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

Instances include:

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

File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L140-L152 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L157 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L180 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L183 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L226 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L258 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L273

File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L189

Splitting require() statements that use && saves gas - (saves 8 gas per &&)

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per &&

File: VoteEscrowDelegation.sol line 239

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

The above should be modified to

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

File: VoteEscrowCore.sol line 538

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

File: VoteEscrowCore.sol line 894

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

File: VoteEscrowCore.sol line 1008

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

Proof The following tests were carried out in remix with both optimization turned on and off

function multiple (uint a) public pure returns (uint){
	require ( a > 1 && a < 5, "Initialized");
	return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

function multiple(uint a) public pure returns (uint){
	require (a > 1 ,"Initialized");
	require (a < 5 , "Initialized");
	return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

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.

See source Use uint256(1) and uint256(2) for true/false to avoid 100 gas and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Instances affected include File: GolomToken.sol line 20

    bool public isAirdropMinted;

File: GolomToken.sol line 21

    bool public isGenesisRewardMinted;

Other instances File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L314 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L341 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L344 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L439 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L440 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L441 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L650 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L651

Comparisons: != is more efficient than > in require/assert statement (6 gas less)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

File: VoteEscrowCore.sol line 927

        require(_value > 0); // dev: need non-zero value

File: VoteEscrowCore.sol line 944

        require(_value > 0); // dev: need non-zero value

File: VoteEscrowCore.sol line 981

        assert(_value > 0); // dev: need non-zero value

use shorter revert strings(less than 32 bytes)

Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.Each extra chunk of bytes past the original 32 incurs an MSTORE which costs 3 gas along with additional overhead for computing memory offset, etc.

File: GolomToken.sol line 24

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L24

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

Other instances to modify File: GolomToken.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L69

File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L181 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L292

File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L73 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L608 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L929 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L945 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L946 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L983

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

Use Custom Errors instead of Revert Strings to save Gas(~50 gas)

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source

File:GolomToken.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L43 File:GolomToken.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L51 File:GolomToken.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L69

File: GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L177 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L211 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L217 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L220 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L222 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L226-L227 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L235 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L285 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L291 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L299 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L359 File: GolamTrader.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L455

File:RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L181 File:RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L184 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L185 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L220 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L292 File: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L309

File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L72 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L73 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L99 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L130 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L186 File: VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L211

File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L538 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L928 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L929 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L945 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L946 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L982 File: VoteEscrowCore.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L983

Use Shift Right/Left instead of Division/Multiplication

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.

File: VoteEscrowDelegation.sol line 150

            uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

The above should be modified to

            uint256 center = upper - (upper - lower) >> 1; // ceil, avoiding overflow

File: VoteEscrowCore.sol line 1049

            uint256 _mid = (_min + _max + 1) / 2;

File: VoteEscrowCore.sol line 1120

            uint256 _mid = (_min + _max + 1) / 2;

See relevant source

x += y costs more gas than x = x + y for state variables

File: VoteEscrowCore.sol line 948

        ++tokenId;

The above should be modified to

tokenId = tokenId + 1;

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

This saves deployement gas

File: VoteEscrowCore.sol line 869

        require(msg.sender == voter);

The above line is repeated ~4 times in the following lines:

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

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

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

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

Using immutable on variables that are only set in the constructor and never after

File: RewardDistributor.sol line 46

    uint256 public startTime; // timestamp at which the contracts need to be activated

File: File: RewardDistributor.sol line 69

        ERC20 public weth;