Golom contest - simon135's results

Lines of code

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

Vulnerability details

Details

A user calls delegate with a fresh totokenId(just minted) his numcheckpoints = 0 Then when using checkpoint[totokenId[numCheckpoints -1] in delegate.writeCheckpoint it dosnt check if nCheckpoints >0 causing a revert because of [0-1] = -1 and its a uint and 0.8 underflow.

Proof of Concept

//@audit this will be zero if new 
  Checkpoint memory oldCheckpoint = checkpoints[toTokenId][
           nCheckpoints - 1
           // 0-1 = -1 in uint (solidity 0.8) it will revert 
       ];

           

when it reverts then delegate reverts causing users not be able to use delegated function.

Recommend Mitigation

put the dos part of the code like this because it can never dos because of nCheckpoints>0.

 if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
     Checkpoint memory oldCheckpoint = checkpoints[toTokenId][
            nCheckpoints - 1
 }
        ];

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L262

Vulnerability details

Impact

To withdraw eth it uses transfer(), this trnansaction will fail inevitably when : -

The withdrwer smart contract does not implement a payable function.

Withdrawer smart contract does implement a payable fallback which uses more than 2300 gas unit

Thw withdrawer smart contract implements a payable fallback function whicn needs less than 2300 gas unit but is called through proxy that raise the call's gas usage above 2300

  if (payAmt > 0) {
            // if royalty has to be paid
//@audit its used in other functions for msg.sender which if smart contract can revert 
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }

if referrer is a smart contract and they have a fallback function that does something then the transfer will revert.

    payEther(o.refererrAmt * amount, referrer);

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L262

Recommended Mitigation Steps

use call instead of transfer and maybe in that case use reentrancy modifier.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L71 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L94 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L168

Vulnerability details

Details

when a user calls getVotes function An attacker can front-run the tx and call the delegate function if needed 499 times (in smart contract that does that) if necessary to push tokenId onto the array. When the attacker calls delegate this is what the attacker does. use his (attackers) tokenId and then the victim totokenId then the same checkpoint and everything else the same so you can get the victims delegatedTokenIds.length = 499. which might cost him a lot (attacker) of gas but the attacker expects it and changes the user's length and then when the user calls getVotes or any other function that uses delegatedTokenIds but in this ex with getvotes will have to loop through 499 times and it's doing a state changing call 499 times which would cause high gas prices and even maybe a total revert because of the overflow of votes if the return of this.balanceOfNFT is a high number.

Proof of concept

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

uint256[] memory delegated = _getCurrentDelegated(tokenId);
        uint256 votes = 0;
        for (uint256 index = 0; index < delegated.length; index++) {
            votes = votes + this.balanceOfNFT(delegated[index]);
        }
        return votes;

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L71 This is where the attacker can start their attack with the explained parameter above.

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

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L94 This is where the attacker sets the victims delegatedTokenIds.length through delegate and once the attacker does this, the victims' functions will fail.

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

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

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }

gas cost website I came up with each call to the balanceOfNfT costs = 7000 gas because sload is cold so it costs 2100 gas and there are 2 of it and returndata, etc. You do those 499700=3,493,000 gas units 100 = basefee 3,493,000 100=349,300,000 (gwei)= $892.34 without talking about tx/market conditions during that time.

Recommend Mitigation

1. one way to solve this , is to make the writeCheckPoint function only callable to the owner of tokenId.
2. I also recommend making delegatedTokenIds < 200 to make the gas lower on the attack ( the cost would be $35 if the length= 199)

NO spdx license which can get you into legal trouble

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol#L1

you should make a non-constant variable undercase to make it more readiable

  uint256 public MIN_VOTING_POWER_REQUIRED = 0;

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

There should be address(0) check

because if there is no address(0) check then important functions that use address(0) for smart contracts calls with revert because address(0) is eoa which dont have code.

  token = _token;

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L53 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L92

you can remove require(blockNumber < block.number);

because the getPriorVotes function has this require check and its also reused in _getPriorDelegated also has this require check. So when _getPriorDelegated gets called in getPriorVotes blockNumber < block.number is already true so it dosnt have to be checked again.

Recommend mitagion

take the require check out of the internal function _getPriorDelegated. https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L130

remove commened out code

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

if admin if comprmised there is no timelock or fail safe for MIN_VOTING_POWER_REQUIRED

ether use timelock and use a dao to mitigate this

My recommend Migration

MIN_VOTING_POWER_REQUIRED have max value that it cant be higher then there wont be as much of an issue for dos

   require(
    // 10> 20 this will revert because admin got compormised 
            this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED,
            "VEDelegation: Need more voting power"
        );

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L261

if there is a malious voter then you can dos all function using

function attach(uint256 _tokenId) external {

        require(msg.sender == voter);
        
        attachments[_tokenId] = attachments[_tokenId] + 1;
    }

dos the functions merge https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L995

make _from into tokenId to make it more readable

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L995

this comment is not needed we are not using vyper

  // Copying and pasting total supply code because Vyper cannot pass by

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1295

make solidity version locked

because if you use one version for testing and another for deploying ,the one for deploying can have bug that the one in testing didnt happen. https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L2

typos

instead, make beginning https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L111 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L111 instead, make facilitated https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L54 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L154 instead, make Execute https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L227 instead, make useful https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L60 instead, make successful https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L211

there can be no timelock if minter = address(0) it bypasses timelock

and if admin byaccident sets minter= address(0) then even not on first call of executeSetMinter there wont be a timelock and if admin gets comprimsed and minter = adddress(0) then you can make any eoa or contract = minter and brake logic of sc and cause the contracts to get redoplyed. https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L73

public functions should be made external if not called in that contract

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L226

its best practice to make 3 indexed or more

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L82

dont use transferFrom because it can fail sielnaclity

erc721.transferFrom can fail with out knowing https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L269

dont make weth contract address static because if the contract changes then you can run into problems

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

this supposed to be the opp and it doesn't check well (deadline > block.timestmap) if the deadline is future deadline then your not going to get the order is deadlined

but if users don't know this then all orders even if the deadline is coming up in that block will be deadlines which should not be the case.

     if (block.timestamp > o.deadline) {

its different in the other examples

don't use a push payment instead use a pull method so you don't have as much dos issues and possible loss of funds

like payther instead make them pay up instead of forcing them to pay or claim rewards in the middle of the function instead let the users claim and pay .

there is a small chance of reentrancy of payether function

because of erc155 safeTransferFrom has a callback a malicious contract can get out of fees but I couldn't come up with an clear exploit so its qa.

o.collection or msg.sender then you on the callback you can reenter because of cross contract reentracy and not pay fees. https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L258

for readability make parm o into order

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

## if epoch gets really big it will cost alot of gas and even might revert.
```javascript 
for (uint256 index = 0; index < epoch; index++) {
            unclaimedepochs[index] = claimed[tokenid][index];
            if (claimed[tokenid][index] == 0) {
                if (index == 0) {
                    rewardEth =
                        rewardEth +
                        (epochTotalFee[0] *
                            ve.balanceOfAtNFT(tokenid, epochBeginTime[1])) /
                        ve.totalSupplyAt(epochBeginTime[1]);
                } else {
                    reward =
                        reward +
                        (rewardStaker[index] *
                            ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /
                        ve.totalSupplyAt(epochBeginTime[index]);
                    rewardEth =
                        rewardEth +
                        (epochTotalFee[index] *
                            ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) /
                        ve.totalSupplyAt(epochBeginTime[index]);
                }
            }
        }

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L354

make the fallback function do something because otherwise users by accident can loose money

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L418-L422

dont use 2 fallback functoins instead use one

just use receive https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L418-L422

on deployment addFee function rewardToken.totalsupply() =0 and in it will revert because you are dividing by 0

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L112

since addFee function is not payable when users to try to weth.deposit the function will revert because you cant give eth

weth.deposit{value: previousEpochFee}();

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L129

make i++ into ++i to remove a memory copy

core/GolomTrader.sol:506:        for (uint256 i = 0; i < proof.length; i++) {
governance/GovernerBravo.sol:240:        for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/GovernerBravo.sol:269:        for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/GovernerBravo.sol:293:        for (uint256 i = 0; i < proposal.targets.length; i++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

state variables that get initialized with 0 are a waste of sstore gas

because of how evm works that uninitialised variable saves 20_000 gas with the same value.

uint256 public MIN_VOTING_POWER_REQUIRED = 0;

vote-escrow/VoteEscrowDelegation.sol:50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;

you can remove require(blockNumber < block.number); to save gas

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L130 because the getPriorVotes function has this require check and its also reused in _getPriorDelegated also has this require check. So when _getPriorDelegated gets called in getPriorVotes blockNumber < block.number is already true so it dosnt have to be checked again.

Recommend mitagion

take the require check out of the internal function _getPriorDelegated.

make _array a memory array instead of storage

  for (uint256 i; i < _array.length; i++) {
            if (_array[i] == _element) {
                _array[i] = _array[_array.length - 1];
                _array.pop();
                break;
            }

every time you use the array its getting it from state which costs alot of gas and if you are looking for an element that is far in the array it can cost so much gas even cause a dos in not carefully every time you sload and if accessed its 100 gas if not 2100 gas and then you store if you get the element which costs gas.

1. sload with array.length and sload with checking if _array[i]== _element which at fist costs 2100 gas and then after the fist 100 gas
2. if you get the element after 50 times then you do sstore costing 20_000 gas. ex: _array=50 uints in storage and element = 49 gas cost storage = (49100) + 5000 +2+100 (sload of pop of last index)=10_000 gas. gas cost memory =( including a sstore because of once you find a match you want the same functionality) (493) + 5000+2+3 =5,152 gas 50% save on gas.

---

This can be used for a malious dos issues or for users wanting to use removeDelgation but dont have enough gas because it costs to much.Users will loose gas money and remove functionality by using storage array for the actually array for looping through. https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L199

waste of memory variable that can save gas

this owner is waste because its not used and we already check above that msg.sender is approved by owner or is owner

  require(
            _isApprovedOrOwner(msg.sender, _tokenId),
            "caller is not owner nor approved"
        );
        //this owner is waste because its not used and we already check above that msg.sender is approved by owner or is owner
        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // Remove token
        _removeTokenFrom(msg.sender, _tokenId);
        //change to emit msg.sender 
        emit Transfer(owner, address(0), _tokenId);

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1229

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. https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L19-L21

Use Custom Errors instead of Revert Strings to save 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) Source 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 are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

vote-escrow/VoteEscrowCore.sol:360:        require(_entered_state == _not_entered);
vote-escrow/VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
vote-escrow/VoteEscrowCore.sol:539:        // Check requirements
vote-escrow/VoteEscrowCore.sol:540:        require(_isApprovedOrOwner(_sender, _tokenId));
vote-escrow/VoteEscrowCore.sol:646:        require(owner != address(0));
vote-escrow/VoteEscrowCore.sol:648:        require(_approved != owner);
vote-escrow/VoteEscrowCore.sol:649:        // Check requirements
vote-escrow/VoteEscrowCore.sol:652:        require(senderIsOwner || senderIsApprovedForAll);
vote-escrow/VoteEscrowCore.sol:869:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:874:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:879:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:884:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:889:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
vote-escrow/VoteEscrowCore.sol:895:        require(_from != _to);
vote-escrow/VoteEscrowCore.sol:896:        require(_isApprovedOrOwner(msg.sender, _from));
vote-escrow/VoteEscrowCore.sol:897:        require(_isApprovedOrOwner(msg.sender, _to));
vote-escrow/VoteEscrowCore.sol:927:        require(_value > 0); // dev: need non-zero value
vote-escrow/VoteEscrowCore.sol:928:        require(_locked.amount > 0, 'No existing lock found');
vote-escrow/VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
vote-escrow/VoteEscrowCore.sol:944:        require(_value > 0); // dev: need non-zero value
vote-escrow/VoteEscrowCore.sol:945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
vote-escrow/VoteEscrowCore.sol:946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
vote-escrow/VoteEscrowCore.sol:982:        require(_locked.amount > 0, 'No existing lock found');
vote-escrow/VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
vote-escrow/VoteEscrowCore.sol:996:        require(_locked.end > block.timestamp, 'Lock expired');
vote-escrow/VoteEscrowCore.sol:997:        require(_locked.amount > 0, 'Nothing is locked');
vote-escrow/VoteEscrowCore.sol:998:        require(unlock_time > _locked.end, 'Can only increase lock duration');
vote-escrow/VoteEscrowCore.sol:999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
vote-escrow/VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
vote-escrow/VoteEscrowCore.sol:1011:        require(block.timestamp >= _locked.end, "The lock didn't expire");
vote-escrow/VoteEscrowCore.sol:1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
vote-escrow/VoteEscrowCore.sol:1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');
vote-escrow/VoteEscrowDelegation.sol:49:    /// @notice minimum voting power required for delegation
vote-escrow/VoteEscrowDelegation.sol:72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
vote-escrow/VoteEscrowDelegation.sol:73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
vote-escrow/VoteEscrowDelegation.sol:99:        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
vote-escrow/VoteEscrowDelegation.sol:130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
vote-escrow/VoteEscrowDelegation.sol:186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
vote-escrow/VoteEscrowDelegation.sol:211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
vote-escrow/VoteEscrowDelegation.sol:220:    //     require(ownerOf(ownerTokenId) == msg.sender, 'VEDelegation: Not allowed');
vote-escrow/VoteEscrowDelegation.sol:239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
vote-escrow/VoteEscrowDelegation.sol:244:        // Check requirements
vote-escrow/VoteEscrowDelegation.sol:245:        require(_isApprovedOrOwner(_sender, _tokenId));
vote-escrow/VoteEscrowDelegation.sol:258:    /// @notice Changes minimum voting power required for delegation
rewards/RewardDistributor.sol:88:        require(msg.sender == trader);
rewards/RewardDistributor.sol:144:            require(epochs[index] < epoch);
rewards/RewardDistributor.sol:158:            require(epochs[index] < epoch);
rewards/RewardDistributor.sol:173:        require(address(ve) != address(0), ' VE not added yet');
rewards/RewardDistributor.sol:181:            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
rewards/RewardDistributor.sol:184:                require(epochs[index] < epoch, 'cant claim for future epochs');
rewards/RewardDistributor.sol:185:  
core/GolomTrader.sol:187:        require(signaturesigner == o.signer, "invalid signature");
core/GolomTrader.sol:221:        require(
core/GolomTrader.sol:233:        require(msg.value >= o.totalAmt * amount + p.paymentAmt, "mgmtm");
core/GolomTrader.sol:236:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:238:        require(o.orderType == 0, "invalid orderType");
core/GolomTrader.sol:246:        require(status == 3, "order not valid");
core/GolomTrader.sol:247:        require(amountRemaining >= amount, "order already filled");
core/GolomTrader.sol:255:            require(amount == 1, "only 1 erc721 at 1 time");
core/GolomTrader.sol:325:        require(
core/GolomTrader.sol:333:        // require eth amt is sufficient
core/GolomTrader.sol:335:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:337:        require(o.orderType == 1);
core/GolomTrader.sol:343:        require(status == 3);
core/GolomTrader.sol:344:        require(amountRemaining >= amount);
core/GolomTrader.sol:347:            require(amount == 1, "only 1 erc721 at 1 time");
core/GolomTrader.sol:373:        require(o.signer == msg.sender);
core/GolomTrader.sol:402:        require(
core/GolomTrader.sol:406:        // require eth amt is sufficient
core/GolomTrader.sol:408:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:410:        require(o.orderType == 2);
core/GolomTrader.sol:416:        require(status == 3);
core/GolomTrader.sol:417:        require(amountRemaining >= amount);
core/GolomTrader.sol:426:            require(amount == 1, "only 1

make require statements with uint >0 make it != to save gas

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

make for loop array.length cached to save gas

vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
core/GolomTrader.sol:506:        for (uint256 i = 0; i < proof.length; i++) {{

you can save gas by re ordering Order struct

      bool isERC721; 
      uint8 v;

make the bool and uint8 under an address because and bool only take up 1 byte and uint8 using 1 bytes an address take up 20 bytes so you have 12 bytes left that you can fit. https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L62 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L55 is saves 2 slots of words and can save 40_000 gas

instead of using abi.encode use abi.encodepacked to save gas

core/GolomTrader.sol:102:            abi.encode(
core/GolomTrader.sol:117:                abi.encode(
core/GolomTrader.sol:138:                abi.encode(

Unchecking arithmetics operations that can’t underflow/overflow

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: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow Unchecked { uint256 total = amount + hasMinted[msg.sender]; }

vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
core/GolomTrader.sol:506:        for (uint256 i = 0; i < proof.length; i++) {

make address(0) into the long way to save gas

make into 0x00000000000000000000000

vote-escrow/VoteEscrowCore.sol:493:        assert(idToOwner[_tokenId] == address(0));
vote-escrow/VoteEscrowCore.sol:508:        idToOwner[_tokenId] = address(0);
vote-escrow/VoteEscrowCore.sol:520:        if (idToApprovals[_tokenId] != address(0)) {
vote-escrow/VoteEscrowCore.sol:522:            idToApprovals[_tokenId] = address(0);
vote-escrow/VoteEscrowCore.sol:646:        require(owner != address(0));
vote-escrow/VoteEscrowCore.sol:679:        assert(_to != address(0));
vote-escrow/VoteEscrowCore.sol:682:        emit Transfer(address(0), _to, _tokenId);
vote-escrow/VoteEscrowCore.sol:861:            assert(IERC20(token).transferFrom(from, address(this), _value));
vote-escrow/VoteEscrowCore.sol:1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
vote-escrow/VoteEscrowCore.sol:1232:        approve(address(0), _tokenId);
vote-escrow/VoteEscrowCore.sol:1235:        emit Transfer(owner, address(0), _tokenId);
vote-escrow/VoteEscrowDelegation.sol:63:        emit Transfer(address(0), address(this), tokenId);
vote-escrow/VoteEscrowDelegation.sol:65:        emit Transfer(address(this), address(0), tokenId);
rewards/RewardDistributor.sol:173:        require(address(ve) != address(0), ' VE not added yet');
rewards/RewardDistributor.sol:177:        address tokenowner = ve.ownerOf(tokenids[0]);
rewards/RewardDistributor.sol:220:        require(address(ve) != address(0), ' VE not added yet');
rewards/RewardDistributor.sol:299:        if (address(ve) == address(0)) {

make revert string less then 32 bytes 1byte a char

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