Golom contest - Sm4rty's results

Lines of code

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

Vulnerability details

Details & Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

• OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
• Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Instances:

contracts/core/GolomTrader.sol:

234:        if (o.isERC721) {
235:            require(amount == 1, "only 1 erc721 at 1 time");
236:            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers.

1. Unused receive() function will lock Ether in contract

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.

Instances:

contracts/core/GolomTrader.sol:461: contracts/rewards/RewardDistributor.sol:315:

contracts/core/GolomTrader.sol:461:    receive() external payable {}
contracts/rewards/RewardDistributor.sol:315:    receive() external payable {}

Referencs

https://code4rena.com/reports/2022-05-sturdy#l-06-unused-receive-function-will-lock-ether-in-contract

---

2. Remove commented out code or function of no use:

There are 2 instances in the file: contracts/vote-escrow/VoteEscrowDeligation.sol:

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

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

Similar Report:

https://code4rena.com/reports/2022-05-sturdy#n-12-remove-commented-out-code

---

3. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

Instances:

There are 4 instances of this issue:

1. contracts/rewards/RewardDistributor.sol:

  70:     event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee);```

2. contracts/vote-escrow/VoteEscrowCore.sol:

   67:     event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
   
  284: event Deposit(
        address indexed provider,
        uint256 tokenId,
        uint256 value,
        uint256 indexed locktime,
        DepositType deposit_type,
        uint256 ts
        
  292:     event Withdraw(address indexed provider, uint256 tokenId, uint256 value, uint256 ts);

3. contracts/vote-escrow/VoteEscrowDelegation.sol:

29:     event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

---

4. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favour of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

contracts/governance/GolomToken.sol:37 contracts/governance/GolomToken.sol:44 contracts/governance/GolomToken.sol:52 contracts/vote-escrow/VoteEscrowCore.sol:950

contracts/governance/GolomToken.sol:37:        _mint(_account, _amount);
contracts/governance/GolomToken.sol:44:        _mint(_airdrop, 150_000_000 * 1e18);
contracts/governance/GolomToken.sol:52:        _mint(_rewardDistributor, 62_500_000 * 1e18);
contracts/vote-escrow/VoteEscrowCore.sol:950:        _mint(_to, _tokenId);

Recommendations:

Use _safeMint() instead of _mint().

---

5. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances:

contracts/core/GolomTrader.sol:

  162:             payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
  255:             ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
  347:             nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
  426:             nftcontract.transferFrom(msg.sender, o.signer, tokenId);
  459:         WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);

contracts/rewards/RewardDistributor.sol:

  151:         rewardToken.transfer(addr, reward);
  165:         rewardToken.transfer(addr, reward);
  208:         rewardToken.transfer(tokenowner, reward);
  209:         weth.transfer(tokenowner, rewardEth);

contracts/vote-escrow/VoteEscrowCore.sol:

   861:             assert(IERC20(token).transferFrom(from, address(this), _value));
  1023:         assert(IERC20(token).transfer(msg.sender, value));

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

---

6.USE A MORE RECENT VERSION OF SOLIDITY

There are many function which are not compatible to run in version less than pragma solidity 0.8.12 ., So try to use updated version of solidity

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

contracts/governance/GolomToken.sol:2:               pragma solidity ^0.8.11;
contracts/vote-escrow/VoteEscrowDelegation.sol:2:               pragma solidity 0.8.11;
contracts/rewards/RewardDistributor.sol:2:               pragma solidity 0.8.11;
contracts/core/GolomTrader.sol:3:               pragma solidity 0.8.11;
contracts/vote-escrow/VoteEscrowCore.sol:2:            pragma solidity 0.8.11;
contracts/vote-escrow/TokenUriHelper.sol:3:             pragma solidity 0.8.11;

---

7. USE OF FLOATING PRAGMA

Recommend using fixed solidity version

Instances

contracts/governance/GolomToken.sol:2

contracts/governance/GolomToken.sol:2:               pragma solidity ^0.8.11;

---

1. Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances:

contracts/rewards/RewardDistributor.sol

173:        require(address(ve) != address(0), ' VE not added yet');
181:        require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
184:        require(epochs[index] < epoch, 'cant claim for future epochs');
185:        require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
220:        require(address(ve) != address(0), ' VE not added yet');
292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

contracts/core/GolomTrader.sol

187:        require(signaturesigner == o.signer, "invalid signature");
232:        require(msg.value >= o.totalAmt * amount + p.paymentAmt, "mgmtm");
237:        require(o.orderType == 0, "invalid orderType");
245:        require(status == 3, "order not valid");
246:        require(amountRemaining >= amount, "order already filled");
254:            require(amount == 1, "only 1 erc721 at 1 time");
345:            require(amount == 1, "only 1 erc721 at 1 time");
424:            require(amount == 1, "only 1 erc721 at 1 time");
548:        require(distributorEnableDate <= block.timestamp, "not allowed");

contracts/governance/GolomToken.sol

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

contracts/vote-escrow/VoteEscrowCore.sol

538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
928:        require(_locked.amount > 0, 'No existing lock found');
929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
982:        require(_locked.amount > 0, 'No existing lock found');
983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
996:        require(_locked.end > block.timestamp, 'Lock expired');
997:        require(_locked.amount > 0, 'Nothing is locked');
998:        require(unlock_time > _locked.end, 'Can only increase lock duration');
999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
1011:        require(block.timestamp >= _locked.end, "The lock didn't expire");
1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

contracts/vote-escrow/VoteEscrowDelegation.sol

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

References:

https://blog.soliditylang.org/2021/04/21/custom-errors/

Remediation:

I suggest replacing revert strings with custom errors.

---

2. Variables: No need to explicitly initialize variables with default values

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

We can use uint number; instead of uint number = 0;

Instance Includes:

contracts/rewards/RewardDistributor.sol

45:    uint256 public epoch = 0;
142:        uint256 reward = 0;
156:        uint256 reward = 0;
175:        uint256 reward = 0;
176:        uint256 rewardEth = 0;
222:        uint256 reward = 0;
223:        uint256 rewardEth = 0;
257:        uint256 reward = 0;
272:        uint256 reward = 0;

contracts/vote-escrow/VoteEscrowCore.sol

735:        uint256 block_slope = 0; // dblock/dt
1042:        uint256 _min = 0;
1113:        uint256 _min = 0;
1133:        uint256 d_block = 0;
1134:        uint256 d_t = 0;
1211:        uint256 dt = 0;

contracts/vote-escrow/VoteEscrowDelegation.sol

50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;
147:        uint256 lower = 0;
170:        uint256 votes = 0;
188:        uint256 votes = 0;

Recommendation:

I suggest removing explicit initializations for default values.

---

3. Require instead of &&

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

Instances include:

contracts/vote-escrow/VoteEscrowCore.sol:538 contracts/vote-escrow/VoteEscrowCore.sol:894 contracts/vote-escrow/VoteEscrowCore.sol:1008 contracts/vote-escrow/VoteEscrowDelegation.sol:239

contracts/vote-escrow/VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
contracts/vote-escrow/VoteEscrowCore.sol:894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
contracts/vote-escrow/VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
contracts/vote-escrow/VoteEscrowDelegation.sol:239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

Mitigation:

Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)

---

4. !=0 instead of >0 for UINT in a require() statement

0 is less efficient than != 0 for unsigned integers (with proof) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

Instances

contracts/vote-escrow/VoteEscrowCore.sol:927 contracts/vote-escrow/VoteEscrowCore.sol:944

contracts/vote-escrow/VoteEscrowCore.sol:927:        require(_value > 0); // dev: need non-zero value
contracts/vote-escrow/VoteEscrowCore.sol:944:        require(_value > 0); // dev: need non-zero value

Reference:

https://twitter.com/gzeon/status/1485428085885640706

Remediation:

I suggest changing > 0 with != 0. Also, please enable the Optimizer.

---

5. An array’s length should be cached to save gas in for-loops

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

Instances:

Links: contracts/core/GolomTrader.sol:415: contracts/rewards/RewardDistributor.sol:143: contracts/rewards/RewardDistributor.sol:157: contracts/rewards/RewardDistributor.sol:180: contracts/rewards/RewardDistributor.sol:183: contracts/vote-escrow/VoteEscrowDelegation.sol:171: contracts/vote-escrow/VoteEscrowDelegation.sol:189: contracts/vote-escrow/VoteEscrowDelegation.sol:199:

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

Remediation:

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead.

---

6. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

Instances:

• contracts/core/GolomTrader.sol

contracts/core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {

• contracts/rewards/RewardDistributor.sol

contracts/rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
contracts/rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
contracts/rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
contracts/rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
contracts/rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
contracts/rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
contracts/rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {

• contracts/vote-escrow/VoteEscrowDelegation.sol

contracts/vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
contracts/vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
contracts/vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {

---

7. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Instances:

contracts/governance/GolomToken.sol:

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

contracts/rewards/RewardDistributor.sol:

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

contracts/vote-escrow/VoteEscrowCore.sol:

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

contracts/vote-escrow/VoteEscrowDelegation.sol:

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

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.****

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

---

8. Shift Right instead of Dividing by 2

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.

I suggest replacing / 2 with >> 1 here:

Instances:

contracts/vote-escrow/VoteEscrowCore.sol:1049 contracts/vote-escrow/VoteEscrowCore.sol:1120 contracts/vote-escrow/VoteEscrowDelegation.sol:150

contracts/vote-escrow/VoteEscrowCore.sol:1049:            uint256 _mid = (_min + _max + 1) / 2;
contracts/vote-escrow/VoteEscrowCore.sol:1120:            uint256 _mid = (_min + _max + 1) / 2;
contracts/vote-escrow/VoteEscrowDelegation.sol:150:            uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-32-shift-right-instead-of-dividing-by-2

---

9. abi.encode() is less efficient than abi.encodePacked()

There are 3 instances of this issue:

- contracts/core/GolomTrader.sol:102

            abi.encode(
                keccak256(
                    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
                ),

- contracts/core/GolomTrader.sol:115:

                abi.encode(
                    keccak256(
                        "payment(uint256 paymentAmt,address paymentAddress)"
                    ),

- contracts/core/GolomTrader.sol:131

                abi.encode(
                    keccak256(
                        "order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)"
                    ),

- contracts/core/GolomTrader.sol:503:

bytes32 computedHash = keccak256(abi.encode(leaf));

Reference:

https://code4rena.com/reports/2022-06-notional-coop#15-abiencode-is-less-efficient-than-abiencodepacked

---

10. 10 ** 18 can be changed to 1e18 and save some gas Submitted by WatchPug, also found by robee

10 ** 18 can be changed to 1e18 to avoid unnecessary arithmetic operation and save gas.s

Instances:

contracts/rewards/RewardDistributor.sol:48 contracts/rewards/RewardDistributor.sol:100

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

---

11. ++I COSTS LESS GAS THAN I++, ESPECIALLY WHEN IT’S USED IN FOR-LOOPS

Instances

contracts/vote-escrow/TokenUriHelper.sol:138

contracts/vote-escrow/TokenUriHelper.sol:138:            digits++;