Golom contest - kyteg's results

Lines of code

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

Vulnerability details

Impact

Creating checkpoints for marking number of votes from a given block on VoteEscrow tokens is impossible, making the vote delegation feature unusable.

Proof of Concept

In contracts/vote-escrow/VoteEscrowDelegation.sol:

All tokens start with numCheckpoints[token] = 0 (from initialisation of the mapping numCheckPoints on line 47). To increment the number of checkpoints, the _writeCheclPoint function must be called. However, the _writeCheckPoint function will revert at line 101 when nCheckPoints = 0 (assigned with the value of numCheckpoints[token]) due to underflow protection. This makes it impossible for any token to have the first checkpoint created, and thus to have any votes delegated to it.

101:        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

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

Tools Used

Manual code inspection

Recommended Mitigation Steps

• Replace line 101 in VoteEscrowDelegation.sol with:

Checkpoint memory oldCheckpoint;

if (nCheckpoints != 0) {
	oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
}

• Create unit tests to test whether votes can be delegated to a token.

Lines of code

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

Vulnerability details

Impact

A check to ensure voting power of a token cannot be delegated more than once is missing, allowing a malicious user to repeatedly call delegate to itself, a single, or multiple tokens to massively amplify the voting power.

Even though a restriction of 500 delegated tokens per token exists, an attacker can simply create new tokens to repeat the process with, to amplify voting power ad infinitum.

Proof of Concept

Imagine an attacker owns token with id=1, with the voting power of the token = 1

1. Attacker calls VoteEscrow.delegate(1, 1) 500 times.

2. The attacker now has 500 delegated votes for the one token.

3. Attacker creates more voting tokens by staking the reward token, and repeats 1-2.

4. The attacker can propose a malicious proposal and execute it with their amplified voting power to take over the contracts and/or drain all funds (eg by changing the reward token contract and exploiting the medium severity vulnerability described in "Reentrancy in RewardDistributer.addFee()").

    /// @notice Explain to an end user what this does
    /// @param tokenId token ID which is being delegated
    /// @param toTokenId token ID to which the {tokenId} is being delegated
    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');


        delegates[tokenId] = toTokenId;
        uint256 nCheckpoints = numCheckpoints[toTokenId];


        if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(tokenId);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
        } else {
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
            _writeCheckpoint(toTokenId, nCheckpoints, array);
        }


        emit DelegateChanged(tokenId, toTokenId, msg.sender);
    }

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

Tools Used

Manual code inspection.

Recommended Mitigation Steps

• Check delegates[tokenId] has not been set (ie equal to 0. Token with tokenId of 0 does not exist, so cannot be delegated to) - require(delegates[tokenId] == 0, 'Already delegated.') on line 74 of VoteEscrowDelegation.sol.
• Set delegates[tokenId] = 0 in VoteEscrow. removeDelegation() (between lines 214 and 215 in VoteEscrowDelegation.sol. )

Lines of code

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

Vulnerability details

Impact

Use of deprecated transfer() function when sending Ether during the processing of an order may result in orders not being filled.

Proof of Concept

transfer() uses a fixed 2300 gas. If the address where funds are transferred to uses more than this to handle the incoming Ether, the order transaction will revert. This limits the protocol to only interact with addresses where gas use upon receiving Ether is less than or equal to 2300 gas.

More information: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

If any of the following take more than 2300 gas to process incoming Ether, the order will revert:

distributor

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

o.exchange.paymentAddress

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

o.prePayment.paymentAddress

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

referrer

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

o.signer

• https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L252-L260
• https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L262-L265

p.paymentAddress

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

msg.sender

• https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L389-L394
• https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L396-L399

Tools Used

Manual code inspection

Recommended Mitigation Steps

Used call instead. For example:

    (bool success, ) = msg.sender.call{amount}("");
    require(success, "Transfer failed.");

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1006-L1030

Vulnerability details

Impact

If a user withdraws staked tokens from VoteEscrow before collecting rewards attached to the staked token NFT, the rewards will be lost.

Proof of Concept

The VoteEscrow.withdraw() function does not check to ensure the staked tokens do not have unclaimed rewards before allowing a withdrawal of staked tokens. If there were rewards attached to the token, these will not be claimable as the NFT to which the rewards were attached is burned.

Imagine a user stakes Golom tokens for a year. The user does not call RewardDistributor. multiStakerClaim() for a year and accumulates a year's worth of reward Golom tokens and wEth. At the end of the year, the user withdraws the staked tokens from voteEscrow thinking that they would receive the accumulated rewards. However, the rewards are not transferred to the user and the user loses a years worth of staking rewards.

Tools Used

Manual code inspection.

Recommended Mitigation Steps

Check with RewardDistributer to ensure the staked tokens do not have any associated rewards attached to it. If there are rewards, the rewards can either be paid out during the withdrawal, or the withdrawal can revert and prompt the user to withdraw rewards first. The VoteEscrowCore.withdraw() function can be overwritten in VoteEscrow.sol to avoid modifying VoteEscrowCore.

Summary

• Low Risk Issues

  • 1. Missing zero address check may lead to loss of staked tokens and accumulated rewards
  • 2. No check to ensure newly minted tokens won't exceed maximum supply
  • 3. Undefined behaviour when a token with delegated votes delegates to another token
  • 4. delagate() can be called on tokens that do not exist
  • 5. Empty receive() and fallback() functions
  • 6. Nonspecific Compiler Version Pragma

• Non-critical Issues

  • 1. Remove commented out code
  • 2. Typo
  • 3. Hardcoded address
  • 4. Unused internal functions can be removed

Low Risk Issues

1. Missing zero address check may lead to loss of staked tokens and accumulated rewards

_transferFrom function in VoteEscrow contract, which transfers the NFT that acts as the key to the staket token vault is missing a zero address check for _to, despite the natspec saying that the function will revert when _to is the zero address. The _transferFrom function is called by all of the external or public function that handles NFT transfer, and these functions themselves also do not check . A user can accidentally transfer a NFT to the zero address, leading to the loss of the NFT and by extension, loss of the staked tokens and unclaimed/future rewards.

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

Recommended mitigation: Check to ensure the _to parameter is not 0x0.

2. No check to ensure newly minted tokens won't exceed maximum supply

Although there is a check to ensure new reward tokens cannot be minted if the total supply is greater than a billion, there is no check to ensure the new total supply after minting the tokens does not exceed a billion.

rewardToken.totalSupply() = 1000000000 * 10**18 - 1

tokenToEmit = 100

=> rewardToken.totalSupply() = 1000000000 * 10**18 + 99 > 1000000000 * 10**18

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

Recommended mitigation: Check to ensure total supply does not exceed 1 billion after minting before minting the new tokens.

>>    if (rewardToken.totalSupply() + tokenToEmit <= 1000000000 * 10**18) {
	        rewardStaker[epoch] = stakerReward;
            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;
            rewardToken.mint(address(this), tokenToEmit);
>>    }

3. Undefined behaviour when a token with delegated votes delegates to another token

Does the final delegated token get all the delegate votes? (this would bypass the 500 delegates limit). Or is this not allowed?

Currently if "token x" delegates voting to "token y" and "token y" delegates voting to "token z", "token z" only receives the voting power of "token y" and the voting power of "token x" is lost unless "token x" calls removeDelegation.

Simplest and most error prone approach would be to disallow tokens with delegated votes to delegate their vote.

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

Recommended mitigation:

• Disallow delegating votes to tokens that already have delegated their vote to another token. Or,
• Disallow delegating votes if a token has received delegate votes from another token.

4. delagate() can be called on tokens that do not exist

The delegate() function in VoteEscrow contract can be called to a _to address that does not yet exist. This may cause unexpected behaviour for users.

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

Recommended mitigation: Check to make sure token exists before delegating to it.

5. Empty receive() and fallback() functions

If Ether is transferred to the contract by accident, it will become locked.

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

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

Recommended mitigation:

• Revert the receive() and fallback() functions when a normal user calls them
  • eg require(msg.sender == address(weth)).

6. Nonspecific Compiler Version Pragma

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

Avoid floating pragmas for non-library contracts.

See: https://github.com/byterocket/c4-common-issues/blob/main/2-Low-Risk.md#l003---unspecific-compiler-version-pragma

Recommended mitigation:

• Use a specific compiler version

Non-critical Issues

1. Remove commented out code

There are 2 instances of this issue:

FILE: contracts/vote-escrow/VoteEscrowDelegation.sol     #1

6   // import {Math} from '@openzeppelin-contracts/utils/math/SafeCast.sol';

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

FILE: contracts/vote-escrow/VoteEscrowDelegation.sol     #2

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

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

2. Typo

FILE: contracts/rewards/RewardDistributor.sol

62:     mapping(uint256 => uint256) public rewardExchange; // reward minted each epoc for exhange

=> exhange -> exchange


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

=> facilated -> facilitated


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

=> dont -> don't


107:             // this assumes atleast 1 trade is done daily??????

=> atleast -> at least


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

=> begiining -> beginning     begining -> beginning


126:                     epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.

=> rewards rewards -> rewards


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

=> there -> their


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

=> facilated -> facilitated


290:     /// @notice Execute's the change trader function

=> Execute's -> Executes

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

FILE: contracts/core/GolomTrader.sol

53:         Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt

=> succesful -> successful


54:         Payment prePayment; // another payment , can be used for royalty, facilating trades

=> facilating -> facilitating


60:         uint256 nonce; // nonce of order usefull for cancelling in bulk 

=> usefull -> useful  


201:     /// @param p any extra payment that the taker of this order wanna send on succesful execution of order

=> succesful -> successful


278:     /// @param p any extra payment that the taker of this order wanna send on succesful execution of order

=> succesful -> successful


288:         ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

=> dont -> don't


329:     ///      to send ether to that address on filling the order, Match an criteria order, ensuring that the supplied proof demonstrates inclusion of the tokenId in the associated merkle root, if root is 0 then any token can be used to fill the order

=> an critical -> a critical


333:     /// @param p any extra payment that the taker of this order wanna send on succesful execution of order

=> succesful -> successful


370:     /// @dev function to settle balances when a bid is filled succesfully

=> succesfully -> successfully


374:     /// @param p any extra payment that the taker of this order wanna send on succesful execution of order

=> succesful -> successful

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

FILE: contracts/vote-escrow/VoteEscrowCore.sol

92:      * - If the caller is not `from`, it must be have been allowed to move this token by either {approve} or {setApprovalForAll}.

=> must be have -> must have


688:     /// @param old_locked Pevious locked amount / end lock time for the user

=> Pevious -> Previous

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

3. Hardcoded address

Hardcoded addresses should be avoided, and set in the contractor.

FILE: contracts/core/GolomTrader.sol

45:    ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

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

4. Unused internal functions can be removed

FILE: contracts/vote-escrow/VoteEscrowDelegation.sol

    function _transferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        address _sender
    ) internal override {

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

Summary

Gas Optimisations

Use custom errors instead of require() to save gas

Custom errors are available from solidity version 0.8.4. The instances below match or exceed that version. Custom errors save about 50 gas per call

There are 44 instances of this issue:

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

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

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

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

FILE: contracts/rewards/RewardDistributor.sol

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

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

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

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

229:    require(amount == 1, 'only 1 erc721 at 1 time');

359:    require(amount == 1, 'only 1 erc721 at 1 time');

455:    require(distributorEnableDate <= block.timestamp, 'not allowed');

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

FILE: contracts/vote-escrow/VoteEscrowCore.sol

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

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

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

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

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

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

Use ++i instead of i++ to save gas

This is especially relevant for the use of i++ in for loops. This saves 6 gas per loop.

See: https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g012---use-prefix-increment-instead-of-postfix-increment-if-possible

There are 12 instances of this issue:

FILE: 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++) {

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

FILE: contracts/rewards/RewardsDistributor.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++) {

173:    for (uint256 index = 0; index < epoch; index++) {

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

FILE: contracts/core/GolomTrader.sol

415:    for (uint256 i = 0; i < proof.length; i++) {

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

FILE: contracts/vote-escrow/TokenUriHelper.sol

138:    digits++;

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

internal functions that are only called once can be inlined to save gas

Depending on the function contents, this will save 20~40 gas by omiting two JUMP operations and stack operations needed for the function call.

_There are 9 instances of this issue:

FILE: 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 {

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

FILE: 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) {

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

Cache array length outside of loop

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

Caching the array length in the stack saves around 3 gas per iteration. To do this, create a variables containing the array length before the loop.

See: https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g002---cache-array-length-outside-of-loop

_There are 8 instances of this issue:

FILE: 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++) {

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

FILE: 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++) {

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

FILE: contracts/core/GolomTrader.sol

415:    for (uint256 i = 0; i < proof.length; i++) {

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

Use != 0 instead of > 0 for a uint

Since the integers are unsigned, != 0 and > 0 are equivalent. Using != 0 is 6 gas per instance cheaper than > 0

See: https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g003---use--0-instead-of--0-for-unsigned-integer-comparison

There are 12 instances of this issue:

FILE: contracts/vote-escrow/VoteEscrowDelegation.sol

78:     if (nCheckpoints > 0) {

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

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

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

FILE: contracts/rewards/RewardDistributor.sol

124:    if (previousEpochFee > 0) {

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

FILE: contracts/core/Golomtrader.sol

152:    if (payAmt > 0) {

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

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

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

FILE: contracts/vote-escrow/VoteEscrowCore.sol

579:    return size > 0;

727:    if (_epoch > 0) {

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

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

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

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

Let the default value be applied to variables initialized to the default value

Letting the default value of 0, false be initialized to variables costs less gas compared to initializing it to these default values.

See: https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g001---dont-initialize-variables-with-default-value

There are 37 instances of this issue:

FILE: contracts/vote-escrow/VoteEscrowDelegation.sol

50:     uint256 public MIN_VOTING_POWER_REQUIRED = 0;

147:    uint256 lower = 0;

170:    uint256 votes = 0;

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

188:    uint256 votes = 0;

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

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

FILE: contracts/rewards/RewardDistributor.sol

45:     uint256 public epoch = 0;

142:    uint256 reward = 0;

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

156:    uint256 reward = 0;

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

175:    uint256 reward = 0;

176:    uint256 rewardEth = 0;

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

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

222:    uint256 reward = 0;

223:    uint256 rewardEth = 0;

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

257:    uint256 reward = 0;

258:    for (uint256 index = 0; index < epoch; index++) {

272:    uint256 reward = 0;

273:    for (uint256 index = 0; index < epoch; index++) {

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

FILE: contracts/core/GolomTrader.sol

415:    for (uint256 i = 0; i < proof.length; i++) {

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

FILE: contracts/vote-escrow/VoteEscrowCore.sol

697:    int128 old_dslope = 0;

698:    int128 new_dslope = 0;

735:    uint256 block_slope = 0; // dblock/dt

745:    for (uint256 i = 0; i < 255; ++i) {

749:    int128 d_slope = 0;

1042:   uint256 _min = 0;

1044:   for (uint256 i = 0; i < 128; ++i) {

1113:   uint256 _min = 0;

1115:   for (uint256 i = 0; i < 128; ++i) {

1133:   uint256 d_block = 0;

1134:   uint256 d_t = 0;

1167:   for (uint256 i = 0; i < 255; ++i) {

1169:   int128 d_slope = 0;

1211:   uint256 dt = 0;

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

Use named return variable

Using a named return variable in the function declaration saves gas.

_There are 7 instances of this issue:

168:    function getVotes(uint256 tokenId) external view returns (uint256) {

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

215     function stakerRewards(uint256 tokenid) public view returns (
216:           uint256,
217:           uint256,
218:           uint256[] memory
219      ){

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

FILE: contracts/rewards/RewardDistributor.sol

254     function traderRewards(address addr) public view returns (
255:          uint256        
256           ){

269     function exchangeRewards(address addr) public view returns (
270:          uint256
271       ){

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

FILE: contracts/vote-escrow/VoteEscrowCore.sol

937     function _create_lock(
938        uint256 _value,
939        uint256 _lock_duration,
940        address _to
941:   ) internal returns (uint256) {

1040:     function _find_block_epoch(uint256 _block, uint256 max_epoch) internal view returns (uint256) {

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