Golom contest - reassor's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L100-L103 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L384

Vulnerability details

Impact

Contract GolomToken uses addFee function of RewardDistributor contract to add rewards for all trades that are being done by users. Function addFees rewards trader and exchange with fees and in addition it converts ether to weth that can be later claimed by VeNFT holders via multiStakerClaim function. The issue is that once rewardToken supply reaches 1 billion of tokens it stops converting ether to weth:

    if (rewardToken.totalSupply() > 1000000000 * 10**18) {
        // if supply is greater then a billion dont mint anything, dont add trades
        return;
    }

This means that new protocol fees will get locked in RewardDistributor contract in form of ether and it will be not possible to claim them by VeNFT holders through multiStakerClaim.

Proof of Concept

rewards/RewardDistributor.sol:

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L100-L103

governance/GolomToken.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to keep converting ether to weth in addFee function even when minting rewards has finished.

Lines of code

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

Vulnerability details

Impact

The Istanbul hardfork increases the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.

Function GolomTrader.payEther is using legacy transfer function, which has a fixed gas stipend and can fail.

The reason behind this is that, after the Istanbul hardfork, any smart contract that uses transfer() or send() is taking a hard dependency on a gas costs by forwarding a fixed amount of gas (2300). This forwards 2300 gas, which may not be enough if the recipient is a contract and the cost of gas changes.

Proof of Concept

core/GolomTrader.sol:

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

Recommended Mitigation Steps

It is recommended to use safe wrapper library, such as the OpenZeppelin Address library’s and its sendValue function that forwards sufficient gas for the transfer regardless of the underlying OPCODE gas costs.

Lines of code

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

Vulnerability details

Impact

Contract GolomTrader uses transferFrom function for transferring NFT between parties. The issue is that NFT transferred to contracts that do not handle ERC721 tokens will be lost. In addition there are a few NFT's example that have logic in the onERC721Received() function, which is only trigged in the safeTransferFrom function.

OpenZeppelin also discourage usage of transferFrom().

Proof of Concept

core/GolomTrader.sol:

• In fillAsk - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L236
• In fillBid - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L301
• In fillCriteriaBid - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L361

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use safeTransferFrom function instead of transferFrom for ERC721 tokens.

Lines of code

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

Vulnerability details

Contract GolomTrader requires sending ether to function fillAsk in order to settle ask order on-chain. Following check is done:

// attached ETH value should be greater than total value of one NFT * total number of NFTs + any extra payment to be given
require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

After that the payment in ether is properly distributed across all parties (such as seller, exchange, distributor etc.). The issue is that if the user sends more ether than o.totalAmt * amount + p.paymentAmt the exceeding part will get locked in the contract forever and cannot be withdrawn since the contract does not implement any administrative functionality for withdrawing ether.

Proof of Concept

core/GolomTrader.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add administrative functionality for withdrawing ether.

List

Low Risk

1. Not following checks-effects-interactions pattern
2. Missing check for status of validateOrder
3. Missing error messages
4. Missing events
5. Missing zero address checks
6. Critical address change

Non-Critical Risk

7. Contract is using unlocked pragma
8. Missing pause functionality
9. Missing/incomplete natspec comments
10. Typos in error messages

1. Not following checks-effects-interactions pattern

Risk

Low

Impact

Multiple functions of GolomToken contract do not follow checks-effects-interactions pattern that might lead to reentrancy attacks.

GolomToken.mintAirdrop:

    function mintAirdrop(address _airdrop) external onlyOwner {
        require(!isAirdropMinted, 'already minted');
        _mint(_airdrop, 150_000_000 * 1e18);
        isAirdropMinted = true;
    }

GolomToken.mintGenesisReward:

    function mintGenesisReward(address _rewardDistributor) external onlyOwner {
        require(!isGenesisRewardMinted, 'already minted');
        _mint(_rewardDistributor, 62_500_000 * 1e18);
        isGenesisRewardMinted = true;
    }

Proof of Concept

governance/GolomToken.sol:

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L42-L46

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to first set the effects such as setting isAirdropMinted and isGenesisRewardMinted = true then perform interactions such as _mint.

2. Missing check for status of validateOrder

Risk

Low

Impact

Function GolomTrader.cancelOrder does not check for return value of validateOrder function which might lead to serious vulnerability if the logic of validateOrder change and it will not throw error when the signature is invalid.

Proof of Concept

core/GolomTrader.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check of status of validateOrder:

    (uint256 status, bytes32 hashStruct, ) = validateOrder(o);
    require(status != 0, 'order not valid');
    filled[hashStruct] = o.tokenAmt + 1;

3. Missing error messages

Risk

Low

Impact

Multiple require statements do not contain error message which makes it difficult to understand why the transaction has reverted.

Proof of Concept

core/GolomTrader.sol:220:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:291:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:293:        require(o.orderType == 1);
core/GolomTrader.sol:295:        require(status == 3);
core/GolomTrader.sol:296:        require(amountRemaining >= amount);
core/GolomTrader.sol:313:        require(o.signer == msg.sender);
core/GolomTrader.sol:342:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
core/GolomTrader.sol:345:            require(msg.sender == o.reservedAddress);
core/GolomTrader.sol:347:        require(o.orderType == 2);
core/GolomTrader.sol:349:        require(status == 3);
core/GolomTrader.sol:350:        require(amountRemaining >= amount);
rewards/RewardDistributor.sol:88:        require(msg.sender == trader);
rewards/RewardDistributor.sol:144:            require(epochs[index] < epoch);
rewards/RewardDistributor.sol:158:            require(epochs[index] < epoch);
vote-escrow/VoteEscrowDelegation.sol:245:        require(_isApprovedOrOwner(_sender, _tokenId));
vote-escrow/VoteEscrowCore.sol:360:        require(_entered_state == _not_entered);
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: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: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:944:        require(_value > 0); // dev: need non-zero value

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add require error messages.

4. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

governance/GolomToken.sol:

• Missing mintAirdrop function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L42-L46
• Missing mintGenesisReward function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L50-L54
• Missing setMinter function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L57-L61
• Missing executeSetMinter function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L65-L72

rewards/RewardDistributor.sol:

• Missing traderClaim function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L141
• Missing exchangeClaim function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L155
• Missing multiStakerClaim function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L172
• Missing changeTrader function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L285
• Missing executeChangeTrader function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L291
• Missing addVoteEscrow function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298
• Missing executeAddVoteEscrow function event - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L308

vote-escrow/VoteEscrowDelegation.sol:

• Missing removeDelegation function event - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L210
• Missing changeMinVotingPower function event - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L260

core/GolomTrader.sol:

• Missing setDistributor function event - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444
• Missing executeSetDistributor function event - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L454

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

5. Missing zero address checks

Risk

Low

Impact

Multiple contracts of Golom protocol do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

governance/GolomToken.sol:

• Missing _governance address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L28
• Missing _minter address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L58

rewards/RewardDistributor.sol:

• Missing _weth, _trader, _token, _governance addresses checks - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L75-L78
• Missing addr address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L141
• Missing addr address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L155
• Missing _trader address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L285
• Missing _voteEscrow address check - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298

vote-escrow/VoteEscrowDelegation.sol:

• Missing _token address check - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L52

core/GolomTrader.sol:

• Missing _governance address check - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L92
• Missing _distributor address check - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

6. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

governance/GolomToken.sol:

• Inheritance through Ownable - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L14

rewards/RewardDistributor.sol:

• Inheritance through Ownable - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L43

vote-escrow/VoteEscrowDelegation.sol:

• Inhertinace through Ownable - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L20

core/GolomTrader.sol:

• Inheritance through Ownable - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L40

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing ownership.

7. Contract is using unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

• governance/GolomToken.sol:pragma solidity ^0.8.11;

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Consider locking compiler version, for example pragma solidity 0.8.11.

8. Missing pause functionality

Risk

Non-Critical

Impact

Contracts are missing pause functionality that could be used in case of security incidents and would block executing selected functions while the contract is paused.

Proof of Concept

governance/GolomToken.sol
vote-escrow/VoteEscrowDelegation.sol
vote-escrow/VoteEscrowCore.sol
rewards/RewardDistributor.sol
core/GolomTrader.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add functionality for pausing contracts and go through all publicly/externally accessible functions to decide which one should be forbidden from running while the contracts are paused.

9. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Multiple contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

governance/GolomToken.sol:

• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L28

rewards/RewardDistributor.sol:

• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L74-L79
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L140-L141
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L154-L155
• Missing @return natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L213-L215
• Missing @return natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L252-L254
• Missing @return natspec - https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L267-L269

vote-escrow/VoteEscrowDelegation.sol:

• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L52
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L94
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L233

core/GolomTrader.sol:

• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L112
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L123
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L127
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L151
• Missing @return natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L164
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L312
• Missing natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L323
• Missing @param tokenId and @param proof natspec - https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L328-L334

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

10. Typos in error messages

Risk

Non-Critical

Impact

Multiple require statements contain typos in their error messages.

Proof of Concept

rewards/RewardDistributor.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to fix typos.

List

1. Obsolete require statements
2. Obsolete code
3. Use scientific notation
4. Cache array length outside of loop
5. Long revert error messages
6. Use custom errors instead of revert strings to save gas
7. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1
8. Obsolete overflow/underflow check
9. No need to explicitly initialize variables with default values
10. Use != 0 instead of > 0 for unsigned integer comparison
11. Pack integer values and reorder struct variables

1. Obsolete require statements

Impact

Contract GolomToken has multiple require statements that are obsolete since the same check is already being done in validateOrder function.

Check in validateOrder that checks if order is already filled:

        if (filled[hashStruct] >= o.tokenAmt) {
            // handles erc1155
            return (2, hashStruct, 0);
        }
        return (3, hashStruct, o.tokenAmt - filled[hashStruct]);

Obsolete require statement checking if order is already filled:

        (uint256 status, bytes32 hashStruct, uint256 amountRemaining) = validateOrder(o);

        require(status == 3, 'order not valid');
        require(amountRemaining >= amount, 'order already filled');  // @audit - obsolete require statement

Proof of Concept

core/GolomTrader.sol:

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L227
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L296
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L350

Recommended Mitigation Steps

It is recommended to remove listed require statement.

2. Obsolete code

Impact

Function GolomTrader.validateOrder implements obsolete code that checks for the same thing. Statement require checks if signaturesigner is equal to o.signer and then if statement is doing the same thing.

        address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Proof of Concept

core/GolomTrader.sol:

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

Recommended Mitigation Steps

It is recommended to remove if statement that always passes since there is require statement before.

3. Use scientific notation

Impact

Contract RewardDistributor is using math exponent calculation to express big numbers. This consumes additional gas and it is better to use scientific notation.

Proof of Concept

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

Recommended Mitigation Steps

It is recommended to use scientific notation, for example: 1e3.

4. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

Recommended Mitigation Steps

It is recommended to cache the array length outside of for loop.

5. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

governance/GolomToken.sol:24:        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
rewards/RewardDistributor.sol:181:            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
rewards/RewardDistributor.sol:292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
rewards/RewardDistributor.sol:309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
vote-escrow/VoteEscrowDelegation.sol:73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
vote-escrow/VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
vote-escrow/VoteEscrowCore.sol:945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
vote-escrow/VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to decrease revert messages to maximum 32 bytes.

6. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

• contracts/governance/GolomToken.sol
• contracts/vote-escrow/VoteEscrowDelegation.sol
• contracts/rewards/RewardDistributor.sol
• contracts/core/GolomTrader.sol
• contracts/vote-escrow/VoteEscrowCore.sol
• contracts/vote-escrow/TokenUriHelper.sol

Recommended Mitigation Steps

It is recommended to add custom errors to listed contracts.

7. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

Recommended Mitigation Steps

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

8. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

Recommended Mitigation Steps

It is recommended to wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }

9. No need to explicitly initialize variables with default values

Impact

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

Proof of Concept

core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:45:    uint256 public epoch = 0;
rewards/RewardDistributor.sol:142:        uint256 reward = 0;
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:156:        uint256 reward = 0;
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:175:        uint256 reward = 0;
rewards/RewardDistributor.sol:176:        uint256 rewardEth = 0;
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:222:        uint256 reward = 0;
rewards/RewardDistributor.sol:223:        uint256 rewardEth = 0;
rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:257:        uint256 reward = 0;
rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:272:        uint256 reward = 0;
rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {
vote-escrow/VoteEscrowDelegation.sol:50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;
vote-escrow/VoteEscrowDelegation.sol:147:        uint256 lower = 0;
vote-escrow/VoteEscrowDelegation.sol:170:        uint256 votes = 0;
vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:188:        uint256 votes = 0;
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowCore.sol:735:        uint256 block_slope = 0; // dblock/dt
vote-escrow/VoteEscrowCore.sol:745:            for (uint256 i = 0; i < 255; ++i) {
vote-escrow/VoteEscrowCore.sol:1042:        uint256 _min = 0;
vote-escrow/VoteEscrowCore.sol:1044:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1113:        uint256 _min = 0;
vote-escrow/VoteEscrowCore.sol:1115:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1133:        uint256 d_block = 0;
vote-escrow/VoteEscrowCore.sol:1134:        uint256 d_t = 0;
vote-escrow/VoteEscrowCore.sol:1167:        for (uint256 i = 0; i < 255; ++i) {
vote-escrow/VoteEscrowCore.sol:1211:        uint256 dt = 0;

Recommended Mitigation Steps

It is recommended to remove explicit initializations with default values.

10. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

core/GolomTrader.sol:152:        if (payAmt > 0) {
core/GolomTrader.sol:250:        if (o.refererrAmt > 0 && referrer != address(0)) {
core/GolomTrader.sol:387:        if (o.refererrAmt > 0 && referrer != address(0)) {
rewards/RewardDistributor.sol:124:            if (previousEpochFee > 0) {
vote-escrow/VoteEscrowDelegation.sol:78:        if (nCheckpoints > 0) {
vote-escrow/VoteEscrowDelegation.sol:103:        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
vote-escrow/VoteEscrowDelegation.sol:119:        return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray;
vote-escrow/VoteEscrowCore.sol:579:        return size > 0;
vote-escrow/VoteEscrowCore.sol:704:            if (old_locked.end > block.timestamp && old_locked.amount > 0) {
vote-escrow/VoteEscrowCore.sol:708:            if (new_locked.end > block.timestamp && new_locked.amount > 0) {
vote-escrow/VoteEscrowCore.sol:727:        if (_epoch > 0) {
vote-escrow/VoteEscrowCore.sol:855:        // value == 0 (extend lock) or value > 0 (add to lock or extend lock)
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:981:        assert(_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');

Recommended Mitigation Steps

It is recommended to use != 0 instead of > 0.

11. Pack integer values and reorder struct variables

Impact

Packing variables into storage slots saves gas.

struct Order {
    address collection; // NFT contract address
    uint256 tokenId; // order for which tokenId of the collection
    address signer; // maker of order address
    uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
    uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
    Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
    Payment prePayment; // another payment , can be used for royalty, facilating trades
    bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
    uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
    uint256 refererrAmt; // amt to pay to the address that helps in filling your order
    bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
    address reservedAddress; // if not address(0) , only this address can fill the order
    uint256 nonce; // nonce of order usefull for cancelling in bulk
    uint256 deadline; // timestamp till order is valid epoch timestamp in secs
    uint8 v;
    bytes32 r;
    bytes32 s;
}

In order to save gas following variables can be packed:

uint256 orderType -> uint8 orderType
uint256 deadline -> uint32 deadline

In addition it is recommended to reorder struct variables:

struct Order {
    address collection; // NFT contract address
    uint256 tokenId; // order for which tokenId of the collection
    address signer; // maker of order address
    uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
    Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
    Payment prePayment; // another payment , can be used for royalty, facilating trades
    uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
    uint256 refererrAmt; // amt to pay to the address that helps in filling your order
    bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
    address reservedAddress; // if not address(0) , only this address can fill the order
    uint256 nonce; // nonce of order usefull for cancelling in bulk
    uint32 deadline; // timestamp till order is valid epoch timestamp in secs
    bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
    uint8 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
    uint8 v;
    bytes32 r;
    bytes32 s;
}

Proof of Concept

core/GolomTrader.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to pack listed integer variables and reorder struct variables.