Golom contest - Jmaxmanblue's results

Lines of code

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

Vulnerability details

Impact

The internal function payEther() makes use of the transfer() function, which uses a fixed stipend of 2300 gas. However, this amount of gas is often not enough to complete a transaction, especially if the receiving contract's fallback function contains any code.

Additionally, this function does not check the return value of transfer at all, meaning that if the transaction ever reverts, it will do so silently. Therefore, replacing the deprecated transfer function will prevent some transfers from failing, and adding a require statement will make it so that the contract no longer allows silent failed transfers.

Since the payEther function is used throughout all the main functions of GolomTrader, any failed transfers means that a participant in the NFT sale, whether the exchange, referrer, or any other address, will not receive their share and the code will continue to run.

Recommended Mitigation Steps

Switch the use of transfer() to call() instead

(bool sent, bytes memory data) = payable(payAddress).call{value: payAmt}("");
require(sent, "Failed to send Ether");

Judge has assessed an item in Issue #463 as Medium risk. The relevant finding follows:

Ignores return value of onERC721Received The try block should include a check to make sure the function returns its selector as specified by IERC721Receiver. While the function may have the same parameters as the standard, its return value may differ, a signal that the receiving smart contract has not properly implemented IERC721Receiver.

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

QA Report

Overall, the codebase was well written but would benefit from a once-over to clean up and make sure all comments are professional.

Unused/empty receive()/fallback() functions

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L459 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L461 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L313 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L315

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

No checks are in place to validate that the address being written is not 0x0

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L59 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L67 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L70 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L81 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L287 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L293

Missing check for address(0x0) when transferring ownership of contract to Governance address

The function _transferOwnership() is used in both constructors to transfer ownership to the governance contract, but this does not inherently check for transferring ownership to the 0x0 address. To mitigate this, the transferOwnership() function can be used instead, which includes a check of the address.

function transferOwnership(address newOwner) public virtual onlyOwner {
        require(newOwner != address(0), "Ownable: new owner is the zero address");
        _transferOwnership(newOwner);
    }

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L29 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L83

Ignores return value of onERC721Received

The try block should include a check to make sure the function returns its selector as specified by IERC721Receiver. While the function may have the same parameters as the standard, its return value may differ, a signal that the receiving smart contract has not properly implemented IERC721Receiver.

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

Assert function should never be used to validate inputs

Properly functioning contracts should never reach a failing assert statement. If the condition checked in the assert() is not actually an invariant, it's suggested that you replace it with a require() statement. When compared to require statements, failing asserts consume all gas instead of refunding.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L493 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L506 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L519 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L666 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L679 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L861 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L980 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L984 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L994 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1010 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1026 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1113 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1209

Non-critical issues

Magic numbers should be documented, use a constant variable instead

Instead of writing 1e18, a constant variable could be used (with no extra gas cost). For instance, something like TOKEN_DECIMALS would provide more clarity.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L44 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L52

Deprecated Documentation

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L151-L156 has outdated documentation The payEther function is not exclusive to pay royalties, nor does it use the royaltyaddress variable.

Incorrect spelling/slang used

Incorrect spelling as well as slang make the code#ess readable and far less professional. "Wanna" is used several times in the documentation https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L201 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L278 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L333 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L374

Including (but not limited to): https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L53 succesful instead of successful https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L54 facilating instead of facilitating https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L57 refererrAmt instead of referrerAmt https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L60 usefull instead of useful https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L201 succesful instead of successful

While I'm unsure if this is intentional, the file Timlock.sol should be renamed to Timelock.sol for clarity, especially as the main contract inside is spelled Timelock as well.

Variables do not follow a capitalization standard

While most variables follow camel case, some are kept all lowercase. For readability and consistency, I recommend to standardize this.

Including (but not limited to): https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L176 _hashOrderinternal instead of _hashOrderInternal https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L176 signaturesigner instead of signatureSigner https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L177 tokenowner instead of tokenOwner https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L224 unclaimedepochs instead of unclaimedEpochs

Unused Variables

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L72 address public governance;

Public functions not called inside the contract should be external

Since none of these functions are called internally, setting their visibility to external will help save gas. Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L98 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L215 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L254 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L155 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L269) https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L172) https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L141 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L284 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L312 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L341 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/TokenUriHelper.sol#L71 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol185

++i costs#ess gas than i++

Even with compiler optimizations enabled, i++ requires the usage of a temporary variable to store the initial value before returning it. However, ++i merely returns the variable after incrementing with no storage needed.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L415 for (uint256 i = 0; i < proof.length; i++) [https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143](https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143 for (uint256 index = 0; index < epochs.length; index++) https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L258 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L273 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/TokenUriHelper.sol#L138 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L745 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1047 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1118 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1170 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol171 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol189 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol199

Increments can be unchecked

To reduce the cost of gas with each increment due to Solidity's default overflow checks, unchecked blocks can be used. For readability, the following example is not done inline.

Previously:

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

After:

for (uint256 i = 0; i < proof.length;) {
    unchecked { ++i; }
}

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L415 for (uint256 i = 0; i < proof.length; i++) [https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143](https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143 for (uint256 index = 0; index < epochs.length; index++) https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L258 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L273 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/TokenUriHelper.sol#L138 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L745 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1047 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1118 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1170 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol171 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol189 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol199

Reduce the size of error messages

Revert strings larger than 32 bytes will increase deployment gas and runtime gas when the condition is met due to the requirement of at least one additional call to mstore, plus additional overhead.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L181 require(tokenowner == ve.ownerOf(tokenids[tindex[]), 'Can only claim for a single Address together') https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L292 require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet') https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L309 require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')

Use custom errors to save further gas

As of Solidity 0.8.4, custom errors provide a cheaper deployment cost and runtime cost when compared with revert strings.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L177 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L211-214 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L222 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L227 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L235 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L299 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L359 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L455 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L43 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L51 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L69 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L173 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L181 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L184 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L185 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L220 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L292 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L309

> 0 is less efficient than != 0 for uint

With the optimizer enabled, != 0 costs less than > 0 inside of a require statement.

Instances include: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L930 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L947