Golom contest - joestakey's results

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

ERC721 token can be lost in fillAsk

PROBLEM

When a user fills an ask order by calling fillAsk, the ERC721.transferFrom method is used to transfer the NFT to the receiver. Should the receiver be a smart contract that is unable to handle ERC721 tokens, this would result in the NFT being locked forever.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId)

TOOLS USED

Manual Analysis

MITIGATION

Consider using a safeTransferFrom method. It is a common approach to avoid using this sort of method to avoid reentrancy, but as it is not mentioned in the readme, this issue is still worth raising.

Lines of code

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

Vulnerability details

Solidity ecrecover is used in the validateOrder function, but not there is no check that the return value is not zero.

If the order maker, ie o.signer, is set as zero, fake orders can pass the validateOrder checks.

Impact

Medium

Proof Of Concept

If you supply an order with invalid o.v, o.r, o.s, ecrecover will return zero, making address signaturesigner = ecrecover(hash, o.v, o.r, o.s) as the zero address. This means the check in the next line will pass for o.signer = address(0), meaning validateOrder passes for fake orders set with o.signer = address(0)

Tools Used

Manual Analysis

Mitigation

Revert if ecrecover returns 0x0. Consider also filtering on the Front-end, preventing orders with 0x0 makers.

Lines of code

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

Vulnerability details

When users fill an ask order by calling fillAsk, the function checks that the msg.value is greater than or equal to the required amount. All the subsequent payments (fees, exchange share, pre payment, order maker) are made based on o.totalAmt, not msg.value. If the amount sent is more than required, there is no way for a user to retrieve this amount.

Impact

Medium

Proof Of Concept

Assume a user calling fillAsk and setting msg.value so that it is strictly greater than o.totalAmt * amount + p.paymentAmt. The payments are done but the difference between msg.value and o.totalAmt * amount + p.paymentAmt is kept in GolomTrader. As there is no withdrawal function in this contract, this amount is locked forever.

Tools Used

Manual Analysis

Mitigation

+ 217:         require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm')
- 217:         require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm')

Another option is to refund the ETH surplus to the user.

QA Report

Table of Contents

• summary

Low

• assert statement should not be used
• Constructors should check the input value
• ERC721 token can be lost in fillAsk
• Filled order can be cancelled
• hash collision with abi.encodePacked
• Lack of escrow can lead to spoofing
• Misleading error string
• Native transfer method should be avoided
• Protocol fees rounding error
• Receive function

Non-critical

• Comment Missing function parameter
• Commented Code
• Constants instead of magic numbers
• Events indexing
• Event should be emitted in setters
• Inconsistent spacing in comments
• Public functions can be external
• Readability of variables
• Require statements should have descriptive strings
• Scientific notation
• Same conditions should have the same revert strings
• Start time too early
• Typos

summary

The main concerns are with the use of deprecated methods - native transfer - and lack of checks in NFT transfer methods - ERC721.transferFrom.

assert statement should not be used

IMPACT

Properly functioning code should never reach a failing assert statement. If it happened, it would indicate the presence of a bug in the contract. A failing assert uses all the remaining gas, which can be financially painful for a user.

SEVERITY

Low

PROOF OF CONCEPT

13 instances include:

VoteEscrowCore.sol

• assert(idToOwner[_tokenId] == address(0))
• assert(idToOwner[_tokenId] == from)
• assert(idToOwner[_tokenId] == _owner)
• assert(_operator != msg.sender)
• assert(_to != address(0))
• assert(IERC20(token).transferFrom(from, address(this), _value))
• assert(_isApprovedOrOwner(msg.sender, _tokenId))
• assert(_value > 0)
• assert(_isApprovedOrOwner(msg.sender, _tokenId))
• assert(_isApprovedOrOwner(msg.sender, _tokenId))
• assert(IERC20(token).transfer(msg.sender, value))
• assert(_block <= block.number)
• assert(_block <= block.number)

TOOLS USED

Manual Analysis

MITIGATION

Replace the assert statements with a require statement or a custom error

Constructors should check the input value

PROBLEM

Constructors should check the value of arguments - ie make revert if it is the zero address. An incorrect address passed would mean the contracts would need to be re-deployed.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomToken.sol

• _transferOwnership(_governance)

GolomTrader.sol

• _transferOwnership(_governance)

RewardDistributor.sol

• weth = ERC20(_weth)
• rewardToken = ERC20(_token)
• _transferOwnership(_governance)

In all contracts, _governance is the admin address that can call key admin functions. weth and rewardToken are state variables, but are never modified in the contracts, meaning an incorrect value passed to them cannot be undone.

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks for these function arguments.

ERC721 token can be lost in fillAsk

PROBLEM

When a user fills an ask order by calling fillAsk, the ERC721.transferFrom method is used to transfer the NFT to the receiver. Should the receiver be a smart contract that is unable to handle ERC721 tokens, this would result in the NFT being locked forever.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId)

TOOLS USED

Manual Analysis

MITIGATION

Consider using a safeTransferFrom method. It is a common approach to avoid using this sort of method to avoid reentrancy, but as it is not mentioned in the readme, this issue is still worth raising.

Filled order can be cancelled

PROBLEM

cancelOrder can be called even for orders that have been filled. While this does not result in any loss of funds, it can be highly misleading to see an event OrderCancelled for an order which has been filled.

SEVERITY

Low

PROOF OF CONCEPT

cancelOrder does an internal call to validateOrder(o), but does not check its first return value, which indicates the order status. Note that it means an expired order can also be cancelled

TOOLS USED

Manual Analysis

MITIGATION

Ensure cancelOrder only works if the order is valid

-314: (, bytes32 hashStruct, ) = validateOrder(o);
+     (uint256 status, bytes32 hashStruct, ) = validateOrder(o);
+     require(status == 3, "order is not valid")

hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• bytes32 hash = keccak256(abi.encodePacked('\x19\x01', EIP712_DOMAIN_TYPEHASH, hashStruct)) hash could potentially be the same for two different orders o and o_ because of this issue.

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

Lack of escrow can lead to spoofing

IMPACT

Order makers can create Ask orders (type 0) with valid signatures, and frontrun fillAsk calls by transferring the o.tokenId to a different wallet. This is frustrating for users as they make the assumption the order is valid, and it also makes it difficult for off-chain tooling to track 'reel' orders.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId)

TOOLS USED

Manual Analysis

MITIGATION

Consider using an escrow. Another possible solution would be to add an extra step in cancelOrder, where any user could cancel an order if ERC721.ownerOf(o.tokenId) != o.signer

Misleading error string

PROBLEM

GolomTrader.sol

• require(amountRemaining >= amount, 'order already filled') The revert string is incorrect. As per the check on the above line, status is 3, meaning the order is still valid and has not been filled (as per these comments).

TOOLS USED

Manual Analysis

MITIGATION

The error string should be amount specified too high

Native transfer method should be avoided

PROBLEM

In GolomTrader, the .transfer() method is used in payEther to handle all ETH transfers in bid and ask functions.

The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

• The contract does not have a payable callback
• The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• payable(payAddress).transfer(payAmt)

TOOLS USED

Manual Analysis

MITIGATION

use .call() instead.

Protocol fees rounding error

PROBLEM

In GolomTrader, There is a division before multiplication in the computation of protocolFees. This leads to a rounding error, meaning some protocolFees are lost.

Because the divisor is only 10,000 - The lost amount is actually very low.

SEVERITY

Low

PROOF OF CONCEPT

GolomTrader.sol

• uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount

Let's take the example of an ERC1155 order with amount = 1,000 and o.totalAmt =2,469.

• protocolFee = ((2,469 * 50)) / 10000 * 1,000
• = 123,450 / 10,000 * 1,000
• = 12,000

the fee is 12,000 while it should be 12,345, ie a 2.8% difference.

Obviously the o.totalAmt is ridiculously low in this example - order of magnitude of 1e-15 ETH - so the fee loss will be much smaller for reel orders. It is however worth noting that the fee loss is inversely proportional to o.totalAmt and proportional to tokenAmt, so the total fee loss will be higher for ERC1155 orders where the amount of tokens is very high. No loss for ERC721 tokens.

TOOLS USED

Manual Analysis

MITIGATION

-381: uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
+381: uint256 protocolfee = o.totalAmt * 50 * amount / 10000;

This would only overflow if o.totalAmt * 50 * amount > 2**256 - 1. 2**256 - 1 being the order of magnitude of 1e77, this is very unlikely.

Receive function

PROBLEM

RewardDistributor.sol has receive() and fallback() functions, but does not have any ETH withdrawal method. Any ETH mistakenly sent to this contract would be locked.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

RewardDistributor.sol

• fallback() external payable
• receive() external payable

TOOLS USED

Manual Analysis

MITIGATION

Remove these functions, or include logic in receive() and fallback(), so that a user that mistakenly sends ETH to the Distributor retrieves it immediately.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

7 instances include:

RewardDistributor.sol

• @ addr, @ epochs
• @ addr, @ epochs

VoteEscrowDelegation.sol

• @ toTokenId,@ nCheckpoints,@ _delegatedTokenIds

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

VoteEscrowDelegation.sol

• this function

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

26 instances include:

GolomToken.sol

• 150_000_000
• 62_500_000

GolomTrader.sol

• 50
• 10000
• 50
• 10000
• 50
• 10000
• 50
• 10000
• 50
• 10000
• 50
• 10000

RewardDistributor.sol

• 1000000000
• 10**18
• 67
• 100
• 33
• 100

VoteEscrowDelegation.sol

• 500

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

2 instances include:

RewardDistributor.sol

• event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee))

VoteEscrowDelegation.sol

• event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

4 instances include:

GolomToken.sol

• function executeSetMinter

GolomTrader.sol

• function executeSetDistributor

RewardDistributor.sol

• function executeChangeTrader
• function executeAddVoteEscrow

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Inconsistent spacing in comments

PROBLEM

Most comments have a space after //, expect for this one instance where there is no space

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• //deadline

TOOLS USED

Manual Analysis

MITIGATION

Change in // deadline

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

GolomTrader.sol

• function fillAsk
• function fillBid
• function cancelOrder
• function fillCriteriaBid

RewardDistributor.sol

• function addFee
• function traderClaim
• function exchangeClaim
• function multiStakerClaim
• function stakerRewards
• function traderRewards
• function exchangeRewards

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Readability of variables

PROBLEM

Follow common practices for variables naming. This also helps with readability:

• use camel case
• capital letters should be for constants only.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

VoteEscrowDelegation.sol

• MIN_VOTING_POWER_REQUIRED
• delegatednft

TOOLS USED

Manual Analysis

MITIGATION

Change to delegatedNft and minVotingPowerRequired

Require statements should have descriptive strings

PROBLEM

Some require statements are missing error strings, which makes it more difficult to debug when the function reverts.

SEVERITY

Non-critical

PROOF OF CONCEPT

29 instances:

GolomTrader.sol

• require(msg.sender == o.reservedAddress)
• require(o.totalAmt * amount >(o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt)
• require(status == 3)
• require(amountRemaining >= amount
• require(o.signer == msg.sender)
• require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt)
• require(msg.sender == o.reservedAddress)
• require(o.orderType == 2
• require(status == 3)
• require(amountRemaining >= amount

RewardDistributor.sol

• require(msg.sender == trader)
• require(epochs[index] < epoch)
• require(epochs[index] < epoch)

VoteEscrowDelegation.sol

• require(_isApprovedOrOwner(_sender, _tokenId))

VoteEscrowCore.sol

• require(_entered_state == _not_entered)
• require(_isApprovedOrOwner(_sender, _tokenId))
• require(owner != address(0))
• require(_approved != owner)
• require(senderIsOwner || senderIsApprovedForAll)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(_from != _to)
• require(_isApprovedOrOwner(msg.sender, _from))
• require(_isApprovedOrOwner(msg.sender, _to))
• require(_value > 0)
• require(_value > 0)

TOOL USED

Manual Analysis

MITIGATION

Add error strings to all require statements.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

10 instances include:

GolomTrader.sol

• 10000
• 10000
• 10000
• 10000
• 10000
• 10000

RewardDistributor.sol

• 1000000000
• 10**18
• 100
• 100

TOOLS USED

Manual Analysis

MITIGATION

Replace the numbers aforementioned with their scientific notation

Same conditions should have the same revert strings

PROBLEM

Same conditions should have the same revert strings

SEVERITY

Non-Critical

PROOF OF CONCEPT

2 instances include:

VoteEscrowCore.sol

• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')
• require(_locked.end > block.timestamp, 'Lock expired')

TOOLS USED

Manual Analysis

MITIGATION

Ensure the same revert string is used

Start time too early

PROBLEM

RewardDistributor start time is set to a time which is anterior to the audit end date.startTime = 1659211200; => 30/7/22 and the contest ends 1/8/22

SEVERITY

Non-Critical

PROOF OF CONCEPT

RewardDistributor.sol.sol

• startTime = 1659211200 corresponds to Sat Jul 30 2022 20:00:00 GMT. The audit is to end on the 1st of August 2022

TOOLS USED

Manual Analysis

MITIGATION

Ensure startTime corresponds to the date you are deploying the contracts

Typos

PROBLEM

There are typos in the contract

SEVERITY

Non-Critical

PROOF OF CONCEPT

26 instances include:

GolomTrader.sol

• facilating should be facilitating
• till should be until - till is not an abbreviation of until.
• if deadline has been - the syntax is incorrect, it is either missing part of the sentence, or should be replaced with if deadline has passed

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos

Gas Report

Table of Contents

High gas savings issues

• Immutable variables save storage
• unused state variable
• Calldata instead of memory for RO function parameters
• Bools for storage are expensive
• Refactor computation
• Constants can be private
• use Assembly for simple setters
• Caching storage variables in local variables to save gas
• Custom errors
• Revert strings length

Low gas savings issues

• Address mappings can be combined in a single mapping
• Array length should not be looked up in every iteration
• Bytes constant are cheaper than string constants
• Constructor parameters should be avoided when possible
• Default value initialization
• Empty blocks should emit an event
• Event fields are redundant
• Inline functions
• Mathematical optimizations
• Modifier instead of duplicate require
• Prefix increments
• Require instead of AND
• Shifting cheaper than division
• Tight variable packing
• unchecked arithmetic
• unnecessary computation

Summary

Several optimizations allow to save significant amounts of gas upon deployment of contracts. The report separates these high savings issues from lower gas saving issues, which are more informational. The most interesting saving is in RewardDistributor.sol, where setting variables that are not modified as immutable allow to save a lot of gas on fillAsk, fillBid and fillCriteriaBid, which are functions invoked by users in GolomTrader. This is the first issue detailed in this report. It is also a very simple change for the team to make Changing require statements into custom errors is also worth considering, as there is many instances (75) across the contracts, and each change would yield high gas savings upon deployments.

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwards, marking it as immutable can save a storage slot.

PROOF OF CONCEPT

4 instances:

GolomTrader.sol

• ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2)

RewardDistributor.sol

• uint256 public startTime
• ERC20 public rewardToken
• ERC20 public weth

TOOLS USED

Manual Analysis

MITIGATION

Mark these variables as immutable.

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 10,068 gas upon deployment for GolomTrader.sol

RewardDistributor

Things are a bit more complex here. If we only make startTime immutable:

• Gas costs before:

• Gas costs after:

This saves 9,829 gas upon deployment for RewardDistributor.sol. But more importantly, this saves 2,103 gas on fillAsk. This is because fillAsk does an external call to addFee, where startTime is fetched.

Now, if we also make rewardToken and weth immutable:

• Gas costs before:

• Gas costs after:

It does cost 10,805 more upon deployment, but fillAsk calls are 4,237 gas cheaper than the original contract. As fillBid and fillCriteriaBid also make an external call to RewardDistributor.addFee, they also benefit from these savings.

Unused state variable

PROBLEM

State variables are expensive as they take storage slots. If they are unused, they should be deleted

PROOF OF CONCEPT

GolomTrader.sol

• address public governance

TOOLS USED

Manual Analysis

MITIGATION

Remove this variable.

GolomTrader

• Gas costs before:

• Gas costs after:

Removing it saves 10,267 gas upon deployment.

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory,but it alleviates the compiler from the abi.decode() step that copies each index of the calldata to the memory index, each iteration costing 60 gas.

PROOF OF CONCEPT

Instances:

GolomTrader.sol

scope: _verifyProof()

• bytes32[] memory proof

RewardDistributor.sol

scope: addFee

• address[2] calldata addr

scope: traderClaim

• uint256[] calldata epochs

scope: exchangeClaim

• uint256[] calldata epochs

scope: multiStakerClaim

• uint256[] calldata tokenids, uint256[] calldata epochs

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 36,479 gas upon deployment for GolomTrader.sol

RewardDistributor

• Gas costs before:

• Gas costs after:

This saves 30,101 gas upon deployment for RewardDistributor.sol

Bools for storage are expensive

IMPACT

Booleans are more expensive than uint256: 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. Compared to using uint256, this costs an extra 100 gas, even one Gsset 20000 gas when changing from 'false' to 'true' - if this is not the first time setting it to true.

PROOF OF CONCEPT

Instances:

GolomToken.sol

• bool public isAirdropMinted
• bool public isGenesisRewardMinted

GolomTrader.sol

• bool isERC721

VoteEscrowCore.sol

• mapping(uint256 => bool) public voted

TOOLS USED

Manual Analysis

MITIGATION

Use uint256(2) and uint256(1) instead of true and false

GolomToken

• Gas costs before:

Total cost including deployment: 2 283 216 gas

• Gas costs after:

Total cost including deployment: 2 279 730 gas

This saves 3,486 gas in total for GolomToken.sol

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 16,814 gas upon deployment for GolomTrader.sol

Refactor computation

IMPACT

Gas can be saved by factoring a function.

(CONST * (a - b)) / a is equivalent to CONST - CONST * b /a

PROOF OF CONCEPT

Instances:

RewardDistributor.sol

• uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) / rewardToken.totalSupply()

TOOLS USED

Manual Analysis

MITIGATION

-            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
-                rewardToken.totalSupply();
+            uint256 tokenToEmit = dailyEmission - dailyEmission * rewardToken.balanceOf(address(ve))/
+                rewardToken.totalSupply() ;

• Gas costs before:

• Gas costs after:

This saves 23,973 gas upon deployment for RewardDistributor.sol. This may however cause imprecision due to dailyEmission * rewardToken.balanceOf(address(ve))/ rewardToken.totalSupply() when rewardToken.balanceOf(address(ve)) is very low.

Constants can be private

IMPACT

Marking constants as private save gas upon deployment, as the compiler does not have to create getter functions for these variables. It is worth noting that a private variable can still be read using either the verified contract source code or the bytecode.

PROOF OF CONCEPT

Instances:

GolomTrader.sol

• bytes32 public immutable EIP712_DOMAIN_TYPEHASH

VoteEscrowCore.sol

• string public constant name = 'veNFT'
• string public constant symbol = 'veNFT'
• string public constant version = '1.0.0'
• uint8 public constant decimals = 18

TOOLS USED

Manual Analysis

MITIGATION

Make the constants private instead of public

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 13,341 gas upon deployment for GolomTrader.sol

use Assembly for simple setters

IMPACT

Where it does not affect readability, using assembly allows to save gas not only on deployment, but also on function calls. This is the case for instance for simple admin setters.

PROOF OF CONCEPT

4 instances:

GolomToken.sol

• pendingMinter = _minter

GolomTrader.sol

• pendingDistributor = _distributor

RewardDistributor.sol

• pendingTrader = _trader
• pendingVoteEscrow = _voteEscrow

MITIGATION

Use sstore to write the state variables new value

-        pendingMinter = _minter;
+        assembly {
+            sstore(pendingMinter.slot, _minter)
+        }

Applying this to all instances mentioned above:

GolomToken

• Gas costs before:

• Gas costs after:

This saves 4,759 gas upon deployment for GolomToken.sol

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 4,747 gas upon deployment for GolomTrader.sol

RewardDistributor

• Gas costs before:

• Gas costs after:

This saves 11,071 gas upon deployment for RewardDistributor.sol

Caching storage variables in local variables to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances:

GolomTrader.sol

scope: fillAsk()

• distributor is read twice:

• payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor))

• distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount)

scope: _settleBalances()

• WETH is read twice:

• WETH.transferFrom(o.signer, address(this), o.totalAmt * amount)

• WETH.withdraw(o.totalAmt * amount)

• distributor is read twice:

• payEther(protocolfee, address(distributor))

• distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee)

RewardDistributor.sol

scope: addFee()

• rewardToken is read 7 times:

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

• uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) / rewardToken.totalSupply()

• uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply()

• rewardToken.mint(address(this), tokenToEmit)

• epoch is read 3 times before it is written:

• if (block.timestamp > startTime + (epoch) * secsInDay)

• uint256 previousEpochFee = epochTotalFee[epoch]

• epoch = epoch + 1

• epoch is read 12 times after it is written:

• rewardStaker[epoch] = stakerReward

• rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100

• rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100

• epochBeginTime[epoch] = block.number

• if (epoch == 1)

• emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee)

• feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee

• feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee

• epochTotalFee[epoch] = epochTotalFee[epoch] + fee;

• ve is read twice:

• uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve))))

• uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply()

• WETH is read twice:

• weth.deposit{value: address(this).balance}()

• weth.deposit{value: previousEpochFee}()

scope: multiStakerClaim

• ve is read 5 or 7 times, depending on this condition:

• require(address(ve) != address(0), ' VE not added yet')

• address tokenowner = ve.ownerOf(tokenids[0])

• require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together')

• ve.balanceOfAtNFT(tokenids[tindex]

• ve.totalSupplyAt(epochBeginTime[1])

• (rewardStaker[epochs[index]] * ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]]))

• ve.totalSupplyAt(epochBeginTime[epochs[index]])

• epochTotalFee[epochs[index]] * ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]]))

• ve.totalSupplyAt(epochBeginTime[epochs[index]])

You should save the return of (ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) and ve.totalSupplyAt(epochBeginTime[epochs[index]]) to save two external calls.

• epochBeginTime[1] is read twice:

• ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[1]))

• ve.totalSupplyAt(epochBeginTime[1])

scope: stakerRewards

• ve is read 3 or 5 times, depending on this condition:

• require(address(ve) != address(0), ' VE not added yet')

• ve.balanceOfAtNFT(tokenid, epochBeginTime[1]))

• ve.totalSupplyAt(epochBeginTime[1]

• ve.balanceOfAtNFT(tokenid, epochBeginTime[index]))

• ve.totalSupplyAt(epochBeginTime[index])

• ve.balanceOfAtNFT(tokenid, epochBeginTime[index]))

• ve.totalSupplyAt(epochBeginTime[index])

You should save the return of ve.balanceOfAtNFT(tokenid, epochBeginTime[index])) and ve.totalSupplyAt(epochBeginTime[index]) to save two external calls.

• epochBeginTime[1] is read twice:

• ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[1]))

• ve.totalSupplyAt(epochBeginTime[1])

scope addVoteEscrow

• ve is read twice:

• if (address(ve) == address(0))

• ve = VE(pendingVoteEscrow)

VoteEscrowCore.sol

scope: approve

• use stack variable owner instead of idToOwner[_tokenId]:
• bool senderIsOwner = (idToOwner[_tokenId] == msg.sender)

scope: _create_lock

948:         ++tokenId;
949:         uint256 _tokenId = tokenId

can be refactored in

948:         uint256 _tokenId = ++tokenId

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables using local variables.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

75 instances:

GolomToken.sol

• require(msg.sender == minter, 'GolomToken: only reward distributor can enable')
• require(!isAirdropMinted, 'already minted')
• require(!isGenesisRewardMinted, 'already minted')
• require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock')

GolomTrader.sol

• require(signaturesigner == o.signer, 'invalid signature')
• require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,'amt not matching')
• require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm')
• require(msg.sender == o.reservedAddress)
• require(o.orderType == 0, 'invalid orderType')
• require(status == 3, 'order not valid')
• require(amountRemaining >= amount, 'order already filled')
• require(amount == 1, 'only 1 erc721 at 1 time')
• require(o.totalAmt * amount >(o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt)
• require(status == 3)
• require(amountRemaining >= amount
• require(amount == 1, 'only 1 erc721 at 1 time')
• require(o.signer == msg.sender)
• require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt)
• require(msg.sender == o.reservedAddress)
• require(o.orderType == 2
• require(status == 3)
• require(amountRemaining >= amount
• require(amount == 1, 'only 1 erc721 at 1 time')
• require(distributorEnableDate <= block.timestamp, 'not allowed')

RewardDistributor.sol

• require(msg.sender == trader)
• require(epochs[index] < epoch)
• require(epochs[index] < epoch)
• require(address(ve) != address(0), ' VE not added yet')
• require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together')
• require(epochs[index] < epoch, 'cant claim for future epochs')
• require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed')
• require(address(ve) != address(0), ' VE not added yet')
• require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')
• require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')

VoteEscrowDelegation.sol

• require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more')
• require(blockNumber < block.number, 'VEDelegation: not yet determined')
• require(blockNumber < block.number, 'VEDelegation: not yet determined')
• require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached')
• require(_isApprovedOrOwner(_sender, _tokenId))

VoteEscrowCore.sol

• require(_entered_state == _not_entered)
• require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached')
• require(_isApprovedOrOwner(_sender, _tokenId))
• require(owner != address(0))
• require(_approved != owner)
• require(senderIsOwner || senderIsApprovedForAll)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(msg.sender == voter)
• require(attachments[_from] == 0 && !voted[_from], 'attached')
• require(_from != _to)
• require(_isApprovedOrOwner(msg.sender, _from))
• require(_isApprovedOrOwner(msg.sender, _to))
• require(_value > 0)
• require(_locked.amount > 0, 'No existing lock found')
• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')
• require(_value > 0)
• require(unlock_time > block.timestamp, 'Can only lock until time in the future')
• require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max')
• require(_locked.amount > 0, 'No existing lock found')
• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')
• require(_locked.end > block.timestamp, 'Lock expired')
• require(_locked.amount > 0, 'Nothing is locked')
• require(unlock_time > _locked.end, 'Can only increase lock duration')
• require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max')
• require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached')
• require(block.timestamp >= _locked.end, "The lock didn't expire")
• require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token')
• require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved')

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in GolomTrader.sol:

Replace

-222: require(o.orderType == 0, 'invalid orderType')
+     if (o.orderType != 0) {
+         revert InvalidOrderType();
+     }

and define the custom error in the contract

+error InvalidOrderType();

GolomTrader

• Gas costs before:

• Gas costs after:

This one require statement changed into a custom error saves 7,567 gas upon deployment for GolomTrader.sol

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include 10 instances:

GolomToken.sol

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

RewardDistributor.sol

• require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together')
• require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')
• require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')

VoteEscrowDelegation.sol

• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')
• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')

VoteEscrowCore.sol

• require(unlock_time > block.timestamp, 'Can only lock until time in the future')
• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

GolomToken

• Gas costs before:

• Gas costs after:

This saves 7,519 gas upon deployment for GolomToken.sol

RewardDistributor

• Gas costs before:

• Gas costs after:

This saves 3,744 gas upon deployment for RewardDistributor.sol

Address mappings can be combined in a single mapping

IMPACT

Combining mappings of address into a single mapping of address to a struct can save a Gssset (20000 gas) operation per mapping combined. This also makes it cheaper for functions reading and writing several of these mappings by saving a Gkeccak256 operation- 30 gas.

PROOF OF CONCEPT

Instances:

RewardDistributor.sol

• mapping(address => mapping(uint256 => uint256)) public feesTrader
• mapping(address => mapping(uint256 => uint256)) public feesExchange

VoteEscrowCore.sol

• mapping(address => uint256) internal ownerToNFTokenCount
• mapping(address => mapping(uint256 => uint256)) internal ownerToNFTokenIdList

TOOLS USED

Manual Analysis

MITIGATION

Combine the address mappings aforementionned in a single address => struct mapping

example:

+58:     struct Fees {
+59:        mapping(uint256 => uint256) trader;
+60:        mapping(uint256 => uint256) exchange;
+61:     }
+62:     mapping(address => Fees) public fees;
-58:     mapping(address => mapping(uint256 => uint256)) public feesTrader; // fees accumulated by address of trader per epoch
-60:     mapping(address => mapping(uint256 => uint256)) public feesExchange; // fees accumulated by exchange of trader per epoch

Array length should not be looked up in every iteration

IMPACT

It wastes gas to read an array's length in every iteration of a for loop, even if it is a memory or calldata array: 3 gas per read.

PROOF OF CONCEPT

Instances:

GolomTrader.sol

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

RewardDistributor.sol

• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 tindex = 0; tindex < tokenids.length; tindex++)
• for (uint256 index = 0; index < epochs.length; index++)

VoteEscrowDelegation.sol

• for (uint256 index = 0; index < delegated.length; index++)
• for (uint256 index = 0; index < delegatednft.length; index++)
• for (uint256 i; i < _array.length; i++)

TOOLS USED

Manual Analysis

MITIGATION

Caching the length in a variable before the for loop

Bytes constant are cheaper than string constants

IMPACT

If the string can fit into 32 bytes, then bytes32 is cheaper than string. string is a dynamically sized-type, which has current limitations in Solidity compared to a statically sized variable.

PROOF OF CONCEPT

Instances:

VoteEscrowCore.sol

• string public constant name = 'veNFT'
• string public constant symbol = 'veNFT'
• string public constant version = '1.0.0'

TOOLS USED

Manual Analysis

MITIGATION

Replace string constant with bytes(1..32) constant

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

4 nstances:

RewardDistributor.sol

• weth = ERC20(_weth)
• trader = _trader
• rewardToken = ERC20(_token)
• startTime = 1659211200

TOOLS USED

Manual Analysis

MITIGATION

Hardcode these variables with their initial value instead of writing it during contract deployment with constructor parameters.

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas for state variables - not for stack variables.

PROOF OF CONCEPT

2 instances:

RewardDistributor.sol

• uint256 public epoch = 0

VoteEscrowDelegation.sol

• uint256 public MIN_VOTING_POWER_REQUIRED = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values in state variables.

RewardDistributor

• Gas costs before:

• Gas costs after:

This saves 2,274 gas upon deployment for RewardDistributor.sol

Empty blocks should emit an event

PROBLEM

Empty blocks in receive or fallback functions should emit an event, or revert. If not, they can simply be removed to save gas upon deployment

PROOF OF CONCEPT

5 instances:

GolomTrader.sol

• fallback() external payable {}
• receive() external payable {}

RewardDistributor.sol

• fallback() external payable {}
• receive() external payable {}

VoteEscrowCore.sol

• try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {}

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in these blocks, or remove them altogether.

Event fields are redundant

PROBLEM

block.timestamp and block.number are added to event information by default, explicitly adding them is a waste of gas.

PROOF OF CONCEPT

1 instance:

VoteEscrowCore.sol

• emit Withdraw(msg.sender, _tokenId, value, block.timestamp)

TOOLS USED

Manual Analysis

MITIGATION

Remove the event field uint256 ts from Withdraw

Inline functions

PROBLEM

When we define internal functions to perform computation:

• The contract’s code size gets bigger
• the function call requires additional stack operations, making it consume more gas than if it was inlined.

When it does not affect readability, it is recommended to inline internal functions that are called only once.

PROOF OF CONCEPT

Instances:

VoteEscrowDelegation.sol

• function _getCurrentDelegated(uint256 tokenId) internal view returns (uint256[] memory)

TOOLS USED

Manual Analysis

MITIGATION

Inline these functions where they are called. Another method is to use the viaIR compiler optimization flag, which helps with inline optimizations.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

18 instances include:

VoteEscrowCore.sol

• ownerToNFTokenCount[_to] += 1
• ownerToNFTokenCount[_from] -= 1
• t_i += WEEK
• last_point.slope += d_slope
• last_point.bias -= last_point.slope * int128(int256(t_i - last_checkpoint))
• _epoch += 1
• last_point.slope += (u_new.slope - u_old.slope)
• last_point.bias += (u_new.bias - u_old.bias)
• old_dslope -= u_old.slope
• old_dslope -= u_new.slope
• new_dslope -= u_new.slope
• _locked.amount += int128(int256(_value))
• last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts))
• block_time += (d_t * (_block - point_0.blk)) / d_block
• upoint.bias -= upoint.slope * int128(int256(block_time - upoint.ts))
• t_i += WEEK
• last_point.bias -= last_point.slope * int128(int256(t_i - last_point.ts))
• last_point.slope += d_slope

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Modifier instead of duplicate require

PROBLEM

When a require statement is used multiple times, it is cheaper in deployment costs to use a modifier instead.

PROOF OF CONCEPT

18 instances include:

GolomTrader.sol

• require(msg.sender == o.reservedAddress)

• require(msg.sender == o.reservedAddress)

• require(o.orderType == 0, 'invalid orderType')

• require(o.orderType == 2

• require(status == 3, 'order not valid')

• require(status == 3)

• require(status == 3)

• require(amountRemaining >= amount, 'order already filled')

• require(amountRemaining >= amount

• require(amountRemaining >= amount

• require(amount == 1, 'only 1 erc721 at 1 time')

• require(amount == 1, 'only 1 erc721 at 1 time')

RewardDistributor.sol

• require(epochs[index] < epoch)

• require(epochs[index] < epoch)

• require(epochs[index] < epoch, 'cant claim for future epochs')

• require(address(ve) != address(0), ' VE not added yet')

• require(address(ve) != address(0), ' VE not added yet')

• require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')

• require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet')

VoteEscrowDelegation.sol

• require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed')

• require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed')

• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')

• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')

• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')

• require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power')

• require(blockNumber < block.number, 'VEDelegation: not yet determined')

• require(blockNumber < block.number, 'VEDelegation: not yet determined')

VoteEscrowCore.sol

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

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

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

• require(_isApprovedOrOwner(_sender, _tokenId))

• require(_isApprovedOrOwner(msg.sender, _from))

• require(_isApprovedOrOwner(msg.sender, _to))

• require(msg.sender == voter)

• require(msg.sender == voter)

• require(msg.sender == voter)

• require(msg.sender == voter)

• require(msg.sender == voter)

• require(_value > 0)

• require(_value > 0)

• require(_locked.amount > 0, 'No existing lock found')

• require(_locked.amount > 0, 'No existing lock found')

• require(unlock_time > block.timestamp, 'Can only lock until time in the future')

• require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max')

• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')

• require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw')

• require(_locked.end > block.timestamp, 'Lock expired')

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops.

PROOF OF CONCEPT

14 instances:

GolomTrader.sol

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

RewardDistributor.sol

• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 tindex = 0; tindex < tokenids.length; tindex++)
• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 index = 0; index < epoch; index++))
• for (uint256 index = 0; index < epoch; index++))
• for (uint256 index = 0; index < epoch; index++))

VoteEscrowDelegation.sol

• for (uint256 index = 0; index < delegated.length; index++)
• for (uint256 index = 0; index < delegatednft.length; index++)
• for (uint256 i; i < _array.length; i++)

VoteEscrowCore.sol

• ownerToNFTokenCount[_to] += 1
• ownerToNFTokenCount[_from] -= 1
• _epoch += 1

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable, and variable += 1 to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas upon function calls. This does cost more upon deployment, meaning there is a trade-off to be decided by the team.

PROOF OF CONCEPT

4 instances:

VoteEscrowDelegation.sol

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

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

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

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

TOOLS USED

Manual Analysis

MITIGATION

Break down the require statements in multiple statements

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

Shifting cheaper than division

IMPACT

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

PROOF OF CONCEPT

3 instances:

VoteEscrowDelegation.sol

• uint256 center = upper - (upper - lower) / 2

VoteEscrowCore.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Replace / 2 with >> 1

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool and uint8 type variables are of size 1 byte, there's two slots in a struct that can get saved by moving them closer to an address

PROOF OF CONCEPT

Instances :

GolomTrader.sol

55:  bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
56:         uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
57:         uint256 refererrAmt; // amt to pay to the address that helps in filling your order
58:         bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
59:         address reservedAddress; // if not address(0) , only this address can fill the order
60:         uint256 nonce; // nonce of order usefull for cancelling in bulk
61:         uint256 deadline; // timestamp till order is valid epoch timestamp in secs
62:         uint8 v;
63:         bytes32 r;
64:         bytes32 s;

TOOLS USED

Manual Analysis

MITIGATION

Rearrange the struct:

-55:  bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
56:         uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
57:         uint256 refererrAmt; // amt to pay to the address that helps in filling your order
58:         bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
59:         address reservedAddress; // if not address(0) , only this address can fill the order
+    bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
+          uint8 v;
60:         uint256 nonce; // nonce of order usefull for cancelling in bulk
61:         uint256 deadline; // timestamp till order is valid epoch timestamp in secs
-62:         uint8 v;
63:         bytes32 r;
64:         bytes32 s;

GolomTrader

• Gas costs before:

• Gas costs after:

This saves 60 gas upon deployment for GolomTrader.sol

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

This is also true for loop increments, which cannot overflow because of a block gas limit.

PROOF OF CONCEPT

Instances:

GolomTrader.sol

• o.tokenAmt - filled[hashStruct] cannot underflow because of this check
• nCheckpoints - 1 cannot underflow because of this check
• uint256 center = upper - (upper - lower) / 2 cannot underflow because of this line
• for (uint256 i = 0; i < proof.length; i++)

RewardDistributor.sol

• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 tindex = 0; tindex < tokenids.length; tindex++)
• for (uint256 index = 0; index < epochs.length; index++)
• for (uint256 index = 0; index < epoch; index++))
• for (uint256 index = 0; index < epoch; index++))
• for (uint256 index = 0; index < epoch; index++))

VoteEscrowDelegation.sol

• for (uint256 index = 0; index < delegated.length; index++)
• for (uint256 index = 0; index < delegatednft.length; index++)
• for (uint256 i; i < _array.length; i++)

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

Stack variables cost gas, remove then when they are redundant.

PROOF OF CONCEPT

GolomTrader.sol

323: function incrementNonce() external nonReentrant {
324:         uint256 newNonce = ++nonces[msg.sender];
325:         emit NonceIncremented(msg.sender, newNonce);
326: }

TOOLS USED

Manual Analysis

MITIGATION

+325:         emit NonceIncremented(msg.sender, ++nonces[msg.sender]);
-324:         uint256 newNonce = ++nonces[msg.sender];
-325:         emit NonceIncremented(msg.sender, newNonce);