Golom contest - TomJ's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L151-L156 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L245 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L248 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L251 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L252-L260 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L262-L265 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L267 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L384 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L385 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L386 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L388 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L389-L394 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L396-L399 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L401

Vulnerability details

Impact

It is recommended to avoid using payable.transfer, since it can cause the transaction to fail when:

1. The claimer smart contract does not have payable function
2. The claimer smart contract does have a payable function but spends more than 2300 gas
3. The claimer smart contract does have a payable function and spends less than 2300 gas but is called through proxy which uses over 2300 gas.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

Function that implements payable.transfer:
GolomTrader.sol:154:            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

Place where this function is executed:
GolomTrader.sol:242:        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
GolomTrader.sol:245:        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
GolomTrader.sol:248:        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
GolomTrader.sol:251:            payEther(o.refererrAmt * amount, referrer);
GolomTrader.sol:252-260:            payEther((o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount, o.signer);
GolomTrader.sol:262-265:            payEther((o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount, o.signer);
GolomTrader.sol:267:        payEther(p.paymentAmt, p.paymentAddress);
GolomTrader.sol:384:        payEther(protocolfee, address(distributor));
GolomTrader.sol:385:        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
GolomTrader.sol:386:        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
GolomTrader.sol:388:            payEther(o.refererrAmt * amount, referrer);
GolomTrader.sol:389-394:            payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount - p.paymentAmt, msg.sender);
GolomTrader.sol:396-399:            payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt, msg.sender);
GolomTrader.sol:401:        payEther(p.paymentAmt, p.paymentAddress);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

Lines of code

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

Vulnerability details

Impact

Use of transferFrom method for ERC721 transfer is discouraged and recommended to use safeTransferFrom whenever possible by OpenZeppelin. This is because transferFrom() cannot check whether the receiving address know how to handle ERC721 tokens.

In the function shown at below PoC, ERC721 token is sent to msg.sender with the transferFrom method. If this msg.sender is a contract and is not aware of incoming ERC721 tokens, the sent token could be locked up in the contract forever.

Reference: https://docs.openzeppelin.com/contracts/3.x/api/token/erc721

Proof of Concept

GolomTrader.sol:236:            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend to call the safeTransferFrom() method instead of transferFrom() for NFT transfers.

Table of Contents

Low Risk Issues

• Missing Zero Address Check
• Require should be used instead of Assert
• Avoid Using ecrecover

Non-critical Issues

• Use fixed compiler versions instead of floating version
• Events Not Emitted for Important State Changes
• Require Statements without Descriptive Revert Strings
• Large Multiples of Ten should use Scientific Notation
• Use Double-Quotes for Strings
• Define Magic Numbers to Constant
• Naming Convention for Constants
• Event is Missing Indexed Fields

 

Low Risk Issues

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally.

PoC

• RewardDistributor.sol: 3 addresses of "weth", "trader" and "rewardToken" https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L74-L85

Mitigation

Add 0-address check for above addresses.

 

Require should be used instead of Assert

Issue

Solidity documents mention that properly functioning code should never reach a failing assert statement and if this happens there is a bug in the contract which should be fixed. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#panic-via-assert-and-error-via-require

PoC

Total of 13 issues found.

VoteEscrowCore.sol:
493:        assert(idToOwner[_tokenId] == address(0));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L493

VoteEscrowCore.sol:
506:        assert(idToOwner[_tokenId] == _from);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L506

VoteEscrowCore.sol:
519:        assert(idToOwner[_tokenId] == _owner);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L519

VoteEscrowCore.sol:
666:        assert(_operator != msg.sender);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L666

VoteEscrowCore.sol:
679:        assert(_to != address(0));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L679

VoteEscrowCore.sol:
861:            assert(IERC20(token).transferFrom(from, address(this), _value));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L861

VoteEscrowCore.sol:
977:        assert(_isApprovedOrOwner(msg.sender, _tokenId));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L977

VoteEscrowCore.sol:
981:        assert(_value > 0); // dev: need non-zero value

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L981

VoteEscrowCore.sol:
991:        assert(_isApprovedOrOwner(msg.sender, _tokenId));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L991

VoteEscrowCore.sol:
1007:        assert(_isApprovedOrOwner(msg.sender, _tokenId));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1007

VoteEscrowCore.sol:
1023:        assert(IERC20(token).transfer(msg.sender, value));

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1023

VoteEscrowCore.sol:
1110:        assert(_block <= block.number);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1110

VoteEscrowCore.sol:
1206:        assert(_block <= block.number);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1206

Mitigation

Replace assert by require.

 

Avoid Using ecrecover

Issue

It is best practice to avoide using ecrecover since the ecrecover EVM opcode allows malleable signatures and thus is vulnerable to reply attacks. Since the contract uses nonce, replay attack seems not possible here but it is still recommended to use recover instead of ecrecover. Also ecrecover returns an empty address when the signature is invalid. So it is also best practice to add 0-address check.

Reference: https://docs.soliditylang.org/en/v0.8.15/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions

PoC

GolomTrader.sol
176:        address signaturesigner = ecrecover(hash, o.v, o.r, o.s);

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

Mitigation

Use the recover function from OpenZeppelin's ECDSA library. Or add zero address check.

require(signaturesigner != address(0))

 

Non-critical Issues

Use fixed compiler versions instead of floating version

Issue

it is best practice to lock your pragma instead of using floating pragma. the use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

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

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Events Not Emitted for Important State Changes

Issue

It is best practice to emit an event when we there is important state changes like update a dynamic array or mapping because it allows monitoring activities with off-chain monitoring tools.

PoC

No event is emitted even though element is removed from a checkpoint.delegatedTokenIds array.

        removeElement(checkpoint.delegatedTokenIds, tokenId);

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L210-L216 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L198-L206

Mitigation

Emit an event when there is important state changes.

 

Require Statements without Descriptive Revert Strings

Issue

It is best practice to include descriptive revert strings for require statement for readability.

PoC

./VoteEscrowDelegation.sol:245:        require(_isApprovedOrOwner(_sender, _tokenId));
./GolomTrader.sol:220:            require(msg.sender == o.reservedAddress);
./GolomTrader.sol:285-288:        require(o.totalAmt * amount > (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt);
./GolomTrader.sol:291:            require(msg.sender == o.reservedAddress);
./GolomTrader.sol:293:        require(o.orderType == 1);
./GolomTrader.sol:295:        require(status == 3);
./GolomTrader.sol:296:        require(amountRemaining >= amount);
./GolomTrader.sol:313:        require(o.signer == msg.sender);
./GolomTrader.sol:342:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
./GolomTrader.sol:345:            require(msg.sender == o.reservedAddress);
./GolomTrader.sol:347:        require(o.orderType == 2);
./GolomTrader.sol:349:        require(status == 3);
./GolomTrader.sol:350:        require(amountRemaining >= amount);
./RewardDistributor.sol:88:        require(msg.sender == trader);
./RewardDistributor.sol:144:            require(epochs[index] < epoch);
./RewardDistributor.sol:158:            require(epochs[index] < epoch);
./VoteEscrowCore.sol:360:        require(_entered_state == _not_entered);
./VoteEscrowCore.sol:540:        require(_isApprovedOrOwner(_sender, _tokenId));
./VoteEscrowCore.sol:646:        require(owner != address(0));
./VoteEscrowCore.sol:648:        require(_approved != owner);
./VoteEscrowCore.sol:652:        require(senderIsOwner || senderIsApprovedForAll);
./VoteEscrowCore.sol:869:        require(msg.sender == voter);
./VoteEscrowCore.sol:874:        require(msg.sender == voter);
./VoteEscrowCore.sol:879:        require(msg.sender == voter);
./VoteEscrowCore.sol:884:        require(msg.sender == voter);
./VoteEscrowCore.sol:889:        require(msg.sender == voter);
./VoteEscrowCore.sol:895:        require(_from != _to);
./VoteEscrowCore.sol:896:        require(_isApprovedOrOwner(msg.sender, _from));
./VoteEscrowCore.sol:897:        require(_isApprovedOrOwner(msg.sender, _to));
./VoteEscrowCore.sol:927:        require(_value > 0); // dev: need non-zero value
./VoteEscrowCore.sol:944:        require(_value > 0); // dev: need non-zero value

Mitigation

Add descriptive revert strings to easier understand what the code is trying to do.

 

Large Multiples of Ten should use Scientific Notation

Issue

Large multiples of ten is hard to read so it is recommended to use scientific notation instead for readability.

PoC

Total of 11 issues found.

./GolomTrader.sol:212:            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
./GolomTrader.sol:242:        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
./GolomTrader.sol:255:                    10000 -
./GolomTrader.sol:263:                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
./GolomTrader.sol:269:        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);
./GolomTrader.sol:381:        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
./RewardDistributor.sol:48:    uint256 constant dailyEmission = 600000 * 10**18;
./RewardDistributor.sol:100:        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
./GolomToken.sol:44:        _mint(_airdrop, 150_000_000 * 1e18);
./GolomToken.sol:52:        _mint(_rewardDistributor, 62_500_000 * 1e18);
./VoteEscrowCore.sol:308:    mapping(uint256 => Point[1000000000]) public user_point_history; // user -> Point[user_epoch]

Mitigation

Change it to scientific notation.

 

[N-06] Use Double-Quotes for Strings

Issue

Strings should be quoted with double-quotes instead of single-quotes. Solidity documents reference: https://docs.soliditylang.org/en/v0.8.15/style-guide.html?highlight=strings#other-recommendations

PoC

Total of 55 issues found.

./VoteEscrowDelegation.sol:72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
./VoteEscrowDelegation.sol:73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
./VoteEscrowDelegation.sol:99:        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
./VoteEscrowDelegation.sol:130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
./VoteEscrowDelegation.sol:186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
./VoteEscrowDelegation.sol:211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
./VoteEscrowDelegation.sol:239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./GolomTrader.sol:177:        require(signaturesigner == o.signer, 'invalid signature');
./GolomTrader.sol:213:            'amt not matching'
./GolomTrader.sol:217:        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
./GolomTrader.sol:222:        require(o.orderType == 0, 'invalid orderType');
./GolomTrader.sol:226:        require(status == 3, 'order not valid');
./GolomTrader.sol:227:        require(amountRemaining >= amount, 'order already filled');
./GolomTrader.sol:235:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:238:            ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
./GolomTrader.sol:299:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:304:            nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId, amount, '');
./GolomTrader.sol:359:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:364:            nftcontract.safeTransferFrom(msg.sender, o.signer, tokenId, amount, '');
./GolomTrader.sol:426:            revert('invalid proof');
./GolomTrader.sol:455:        require(distributorEnableDate <= block.timestamp, 'not allowed');
./RewardDistributor.sol:173:        require(address(ve) != address(0), ' VE not added yet');
./RewardDistributor.sol:181:            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
./RewardDistributor.sol:184:                require(epochs[index] < epoch, 'cant claim for future epochs');
./RewardDistributor.sol:185:                require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
./RewardDistributor.sol:220:        require(address(ve) != address(0), ' VE not added yet');
./RewardDistributor.sol:292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
./RewardDistributor.sol:309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
./GolomToken.sol:24:        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
./GolomToken.sol:28:    constructor(address _governance) ERC20('Golom', 'GOL') ERC20Permit('Golom') {
./GolomToken.sol:43:        require(!isAirdropMinted, 'already minted');
./GolomToken.sol:51:        require(!isGenesisRewardMinted, 'already minted');
./GolomToken.sol:69:            require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');
./VoteEscrowCore.sol:317:    string public constant name = 'veNFT';
./VoteEscrowCore.sol:318:    string public constant symbol = 'veNFT';
./VoteEscrowCore.sol:319:    string public constant version = '1.0.0';
./VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./VoteEscrowCore.sol:608:                    revert('ERC721: transfer to non ERC721Receiver implementer');
./VoteEscrowCore.sol:634:        safeTransferFrom(_from, _to, _tokenId, '');
./VoteEscrowCore.sol:894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
./VoteEscrowCore.sol:928:        require(_locked.amount > 0, 'No existing lock found');
./VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
./VoteEscrowCore.sol:945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
./VoteEscrowCore.sol:946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
./VoteEscrowCore.sol:982:        require(_locked.amount > 0, 'No existing lock found');
./VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
./VoteEscrowCore.sol:996:        require(_locked.end > block.timestamp, 'Lock expired');
./VoteEscrowCore.sol:997:        require(_locked.amount > 0, 'Nothing is locked');
./VoteEscrowCore.sol:998:        require(unlock_time > _locked.end, 'Can only increase lock duration');
./VoteEscrowCore.sol:999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
./VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./VoteEscrowCore.sol:1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
./VoteEscrowCore.sol:1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');
./TokenUriHelper.sol:9:    bytes internal constant TABLE = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
./TokenUriHelper.sol:14:        if (len == 0) return '';

Mitigation

Change above single-quotes to double-quotes.

 

[N-05] Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

1. Magic Number: 50

./GolomTrader.sol:212:            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
./GolomTrader.sol:242:        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
./GolomTrader.sol:254:                    (o.totalAmt * 50) /
./GolomTrader.sol:263:                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
./GolomTrader.sol:269:        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);
./GolomTrader.sol:381:        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

2. Magic Number: 500

./VoteEscrowDelegation.sol:99:        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

3. Magic Number: 10000

./GolomTrader.sol:212:            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
./GolomTrader.sol:242:        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
./GolomTrader.sol:255:                    10000 -
./GolomTrader.sol:263:                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
./GolomTrader.sol:269:        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);
./GolomTrader.sol:381:        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

4. Magic Number: 1000000000

./RewardDistributor.sol:100:        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
./VoteEscrowCore.sol:308:    mapping(uint256 => Point[1000000000]) public user_point_history; // user -> Point[user_epoch]

5. Magic Number: 62_500_000

./GolomToken.sol:52:        _mint(_rewardDistributor, 62_500_000 * 1e18);

6. Magic Number: 150_000_000

./GolomToken.sol:44:        _mint(_airdrop, 150_000_000 * 1e18);

7. Magic Number: 1e18

./GolomToken.sol:44:        _mint(_airdrop, 150_000_000 * 1e18);
./GolomToken.sol:52:        _mint(_rewardDistributor, 62_500_000 * 1e18);

Mitigation

Define magic numbers to constant.

 

Naming Convention for Constants

Issue

Constants should be named with all capital letters with underscores separating words.

Solidity documentation: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#constants

PoC

Below 8 constant variable should follow constants naming convention best practice.

./RewardDistributor.sol:48:    uint256 constant dailyEmission = 600000 * 10**18;
./RewardDistributor.sol:57:    uint256 constant secsInDay = 24 * 60 * 60;
./VoteEscrowCore.sol:317:    string public constant name = 'veNFT';
./VoteEscrowCore.sol:318:    string public constant symbol = 'veNFT';
./VoteEscrowCore.sol:319:    string public constant version = '1.0.0';
./VoteEscrowCore.sol:320:    uint8 public constant decimals = 18;
./VoteEscrowCore.sol:356:    uint8 internal constant _not_entered = 1;
./VoteEscrowCore.sol:357:    uint8 internal constant _entered = 2;

Below 2 variable should not be named with all capital letters since it is not constants.

./VoteEscrowDelegation.sol:50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;
./GolomTrader.sol:41:    bytes32 public immutable EIP712_DOMAIN_TYPEHASH;

Mitigation

If the variable is constant, name it with all capital letters. If the variable is not constant, do not name it with all capital letters.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./VoteEscrowDelegation.sol:29:    event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);
./GolomTrader.sol:79:    event NonceIncremented(address indexed maker, uint256 newNonce);
./RewardDistributor.sol:70:    event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee);
./VoteEscrowCore.sol:67:    event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
./VoteEscrowCore.sol:284:    event Deposit(address indexed provider, uint256 tokenId, uint256 value, uint256 indexed locktime, DepositType deposit_type, uint256 ts);
./VoteEscrowCore.sol:292:    event Withdraw(address indexed provider, uint256 tokenId, uint256 value, uint256 ts);
./VoteEscrowCore.sol:293:    event Supply(uint256 prevSupply, uint256 supply);

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

Total of 21 types of gas optimization was found.

• Should Use Unchecked Block where Over/Underflow is not Possible
• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Use Already Defined Variable
• Order Struct Member of orderType can be uint8
• Struct Member can be Packed into Fewer Storage Slots
• Unchanging State Variable Should be Immutable
• Change Function Visibility Public to External
• Internal Function Called Only Once can be Inlined
• Use Bit Shifting Instead of Multiplication/Division of 2
• Duplicate require() Checks Should be a Modifier or a Function
• Use require instead of &&
• Use Calldata instead of Memory for Read Only Function Parameters
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Unnecessary Default Value Initialization
• Store Array's Length as a Variable
• ++i Costs Less Gas than i++
• != 0 costs less gas than > 0
• Empty Blocks Should Emit Something or be Removed
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

PoC

1. delegate() of VoteEscrowDelegation.sol

Wrap line 79 with unchecked since underflow is not possible due to line 78 check
78:        if (nCheckpoints > 0) {
79:            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L78-L79

Mitigation

Wrap the code with uncheck like described in above PoC.

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

Total of 12 issues were found.

1. Cache epoch variable of addFee() of RewardDistributor.sol

Since there is state change of epoch in the middle of the function, we can cache it 2 times like beforeEpoch and afterEpoch. 12 SLOADs to 2 SLOAD, 2 MSTORE and 12 MLOADs (not including the state change epoch of line 118)

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L106 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L117-L121 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L123 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L125 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L132 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L134-L136

Change the code to cache it like the following:
(line 106-138)

         uint256 beforeEpoch = epoch //@ 1 SLOAD and 1 MSTORE
         if (block.timestamp > startTime + (beforeEpoch) * secsInDay) { //@ 1 MLOAD
            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

            uint256 previousEpochFee = epochTotalFee[beforeEpoch]; //@ 1 MLOAD
            epoch = epoch + 1; //@ 1 MLOAD

            uint256 afterEpoch = epoch //@ 1 SLOAD and 1 MSTORE

            rewardStaker[afterEpoch] = stakerReward; //@ 1 MLOAD
            rewardTrader[afterEpoch] = ((tokenToEmit - stakerReward) * 67) / 100; //@ 1 MLOAD
            rewardExchange[afterEpoch] = ((tokenToEmit - stakerReward) * 33) / 100; //@ 1 MLOAD
            rewardToken.mint(address(this), tokenToEmit);
            epochBeginTime[afterEpoch] = block.number; //@ 1 MLOAD
            if (previousEpochFee > 0) {
                if (afterEpoch == 1){ //@ 1 MLOAD
                    epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                    weth.deposit{value: address(this).balance}();  
                }else{
                    weth.deposit{value: previousEpochFee}();
                }
            }
            emit NewEpoch(afterEpoch, tokenToEmit, stakerReward, previousEpochFee); //@ 1 MLOAD
        }
        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee; //@ 1 MLOAD
        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee; //@ 1 MLOAD
        epochTotalFee[epoch] = epochTotalFee[epoch] + fee; //@ 1 MLOAD
        return;
    }       

2. Cache epoch variable of stakerRewards() of RewardDistributor.sol 2 SLOADs or more to 1 SLOAD, 1 MSTORE and 2 MLOADs or more

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L224 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L226

3. Cache epochBeginTime[1] variable of multiStakerClaim() of RewardDistributor.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L191-L192

4. Cache epochBeginTime[epochs[index]] variable of multiStakerClaim() of RewardDistributor.sol 4 SLOADs to 1 SLOAD, 1 MSTORE and 4 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L197-L198 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L202-L203

5. Cache epochBeginTime[1] variable of stakerRewards() of RewardDistributor.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L233-L234

6. Cache epochBeginTime[index] variable of stakerRewards() of RewardDistributor.sol 4 SLOADs to 1 SLOAD, 1 MSTORE and 4 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L239-L240 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L244-L245

7. Cache claimed variable of stakerRewards() of RewardDistributor.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L227-L228

8. Cache rewardToken variable of addFee() of RewardDistributor.sol 7 SLOADs to 1 SLOAD, 1 MSTORE and 7 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L100 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L112-L114 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L122

9. Cache weth variable of addFee() of RewardDistributor.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

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

10. Cache user_point_history[_tokenId] variable of _balanceOfAtNFT() of VoteEscrowCore.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1121 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1128

11. Cache filled[hashStruct] variable of validateOrder() of GolomTrader.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L193

12. Cache distributor variable of _settleBalances() of GolomTrader.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs

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

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

Total of 6 issues found

1. uepoch of get_last_user_slope() VoteEscrowCore.sol

• remove 'uepoch' variable https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L375-L378 Delete line 376 and replace line 377 to line shown below

        return user_point_history[_tokenId][user_point_epoch[_tokenId]].slope;

2. spenderIsOwner, spenderIsApproved, spenderIsApprovedForAll of _isApprovedOrOwner() VoteEscrowCore.sol

• remove 'spenderIsOwner', 'spenderIsApproved', 'spenderIsApprovedForAll' variables https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L437-L443 Delete line 439-441 and replace line 442 to line shown below

        return owner == _spender || _spender == idToApprovals[_tokenId] || (ownerToOperators[owner])[_spender];

3. spenderIsOwner, spenderIsApprovedForAll of approve() VoteEscrowCore.sol

• remove 'spenderIsOwner', 'spenderIsApprovedForAll' variables https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L643-L656 Delete line 650-651 and replace line 652 to line shown below

        require((idToOwner[_tokenId] == msg.sender) || (ownerToOperators[owner])[msg.sender]);

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Use Already Defined Variable

Issue

Use already defined variable rather than reading the storage variable again and wasting gas.

PoC

1. approve() of VoteEscrowCore.sol Use cached variable of owner instead of reading the storage variable https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L650

Change idToOwner[_tokenId] to owner

650:        bool senderIsOwner = (owner == msg.sender);

Mitigation

Use the already defined variable like shown in above PoC.

 

Order Struct Member of orderType can be uint8

Issue

Order struct member of orderType uses only number of 0, 1 and 2 according to the comment.

uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root

Since it uses only 3 number, it can be declared as uint8 type, which packs the struct size and will reduce one storage slot.

PoC

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

uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root

Change it to

uint8 orderType;

This will get packed with address signer, which will reduce one storage slot.

Mitigation

Follow above PoC and reduce one storage slot.

 

Struct Member can be Packed into Fewer Storage Slots

Issue

The order of struct member can be reordered in a way to reduce the usage amount of storage slots.

Reference from solidity documentation:
Finally, in order to allow the EVM to optimize for this, ensure that you try to order your storage 
variables and struct members such that they can be packed tightly. For example, declaring your 
storage variables in the order of uint128, uint128, uint256 instead of uint128, uint256, uint128, 
as the former will only take up two slots of storage whereas the latter will take up three.

https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html#layout-of-state-variables-in-storage

PoC

Total of 1 issue found.

1. struct "Order" of GolomTrader.sol Originally it uses 17 slots but this can be reduced to 15 slots by reordering the struct member like below. Pack bool(1byte) and uint8(1byte) to address(20bytes) to fit all 3 elements into 1 slot(32bytes).

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

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

Mitigation

Reorder struct member like shown in above PoC.

 

Unchanging State Variable Should be Immutable

Issue

State variable that is only set in the constructor and can't be changed afterwards, should be declared as immutable.

PoC

1. startTime variable of RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L46

2. rewardToken variable of RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L68

3. weth variable of RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L69

 

Change Function Visibility Public to External

Issue

If the function is not called internally, it is cheaper to set your function visibility to external instead of public.

PoC

Total of 12 issues found.

VoteEscrowDelegation.sol:getPriorVotes() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L185-L193

GolomTrader.sol:fillAsk() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L203-L271

GolomTrader.sol:fillBid() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L279-L308

GolomTrader.sol:cancelOrder() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L312-L317

GolomTrader.sol:fillCriteriaBid() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L334-L368

RewardDistributor.sol:addFee() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L98-L103

RewardDistributor.sol:traderClaim() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L141-L152

RewardDistributor.sol:exchangeClaim() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L155-L166

RewardDistributor.sol:multiStakerClaim() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L172-L210

RewardDistributor.sol:stakerRewards() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L215-L250

RewardDistributor.sol:traderRewards() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L254-L265

RewardDistributor.sol:exchangeRewards() https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L269-L280

Mitigation

Change the function visibility to external.

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 9 issues found.

1. _getCurrentDelegated() of VoteEscrowDelegation.sol _getCurrentDelegated function called only once at line 169 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L116-L120 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L169

2. _getPriorDelegated() of VoteEscrowDelegation.sol _getPriorDelegated function called only once at line 187 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L129-L161 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L187

3. removeElement() of VoteEscrowDelegation.sol removeElement function called only once at line 214 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L198-L206 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L214

4. _addTokenToOwnerList() of VoteEscrowCore.sol _addTokenToOwnerList function called only once at line 497 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L452-L457 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L497

5. _removeTokenFromOwnerList() of VoteEscrowCore.sol _removeTokenFromOwnerList function called only once at line 510 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L462-L487 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L510

6. _clearApproval() of VoteEscrowCore.sol _clearApproval function called only once at line 542 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L517-L524 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L542

7. _isContract() of VoteEscrowCore.sol _isContract function called only once at line 542 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L571-L580 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L602

8. _mint() of VoteEscrowCore.sol _mint function called only once at line 950 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L677-L684 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L950

9. _balanceOfAtNFT() of VoteEscrowCore.sol _balanceOfAtNFT function called only once at line 1157 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1107-L1154 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1157

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Use Bit Shifting Instead of Multiplication/Division of 2

Issue

The MUL and DIV opcodes cost 5 gas but SHL and SHR only costs 3 gas. Since MUL/DIV and SHL/SHR result the same, use cheaper bit shifting.

PoC

Total of 3 issues found.

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

Mitigation

Use bit shifting instead of multiplication/division. Example:

Bad:
uint256 center = upper - (upper - lower) / 2; 
Good:
uint256 center = upper - (upper - lower) >> 2; 

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

Total of 14 issues found.

./GolomTrader.sol:220:            require(msg.sender == o.reservedAddress);
./GolomTrader.sol:291:            require(msg.sender == o.reservedAddress);
./GolomTrader.sol:345:            require(msg.sender == o.reservedAddress);

./GolomTrader.sol:295:        require(status == 3);
./GolomTrader.sol:349:        require(status == 3);

./GolomTrader.sol:296:        require(amountRemaining >= amount);
./GolomTrader.sol:350:        require(amountRemaining >= amount);

./RewardDistributor.sol:144:            require(epochs[index] < epoch);
./RewardDistributor.sol:158:            require(epochs[index] < epoch);

./VoteEscrowCore.sol:869:        require(msg.sender == voter);
./VoteEscrowCore.sol:874:        require(msg.sender == voter);
./VoteEscrowCore.sol:879:        require(msg.sender == voter);
./VoteEscrowCore.sol:884:        require(msg.sender == voter);
./VoteEscrowCore.sol:889:        require(msg.sender == voter);

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

./VoteEscrowDelegation.sol:72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
./VoteEscrowDelegation.sol:211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

./VoteEscrowDelegation.sol:130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
./VoteEscrowDelegation.sol:186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');

./GolomTrader.sol:235:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:299:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:359:            require(amount == 1, 'only 1 erc721 at 1 time');

./RewardDistributor.sol:173:        require(address(ve) != address(0), ' VE not added yet');
./RewardDistributor.sol:220:        require(address(ve) != address(0), ' VE not added yet');

./VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

./VoteEscrowCore.sol:928:        require(_locked.amount > 0, 'No existing lock found');
./VoteEscrowCore.sol:982:        require(_locked.amount > 0, 'No existing lock found');

./VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
./VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

./VoteEscrowCore.sol:946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
./VoteEscrowCore.sol:999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Use require instead of &&

Issue

When there are multiple conditions in require statement, break down the require statement into multiple require statements instead of using && can save gas.

PoC

Total of 4 issues found.

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

Mitigation

Break down into several require statement. For example,

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

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

Total of 15 issues found.

./VoteEscrowDelegation.sol:97:        uint256[] memory _delegatedTokenIds
./GolomTrader.sol:127:    function _hashOrderinternal(Order calldata o, uint256[2] memory extra) private pure returns (bytes32) {
./GolomTrader.sol:412:        bytes32[] memory proof
./RewardDistributor.sol:98:    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
./RewardDistributor.sol:141:    function traderClaim(address addr, uint256[] memory epochs) public {
./RewardDistributor.sol:155:    function exchangeClaim(address addr, uint256[] memory epochs) public {
./RewardDistributor.sol:172:    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
./RewardDistributor.sol:218:            uint256[] memory
./VoteEscrowCore.sol:598:        bytes memory _data
./VoteEscrowCore.sol:605:                bytes memory reason
./VoteEscrowCore.sol:692:        LockedBalance memory old_locked,
./VoteEscrowCore.sol:693:        LockedBalance memory new_locked
./VoteEscrowCore.sol:837:        LockedBalance memory locked_balance,
./VoteEscrowCore.sol:1164:    function _supply_at(Point memory point, uint256 t) internal view returns (uint256) {
./TokenUriHelper.sol:12:    function encode(bytes memory data) internal pure returns (string memory) {

Mitigation

Change memory to calldata

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 5 issues found.

./GolomTrader.sol:62:        uint8 v;
./VoteEscrowCore.sol:320:    uint8 public constant decimals = 18;
./VoteEscrowCore.sol:356:    uint8 internal constant _not_entered = 1;
./VoteEscrowCore.sol:357:    uint8 internal constant _entered = 2;
./VoteEscrowCore.sol:358:    uint8 internal _entered_state = 1;

Mitigation

I suggest using uint256 instead of anything smaller or downcast where needed.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 37 issues found.

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

Mitigation

I suggest removing default value initialization. For example,

• uint256 public MIN_VOTING_POWER_REQUIRED;
• for (uint256 index; index < delegated.length; index++) {

 

Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

Total of 2 issues found.

./VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
./VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {

Mitigation

Store array's length as a variable before looping it.

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

Total of 12 issues found.

./VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
./VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
./VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
./GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
./RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
./RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
./RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
./RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
./RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
./RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
./RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {
./TokenUriHelper.sol:138:            digits++;

Mitigation

Change it to postfix increments/decrements. For example,

for (uint256 index = 0; index < delegated.length; ++index) {

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

Total of 2 issues found.

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

Mitigation

I suggest changing > 0 to != 0

require(_value != 0); // dev: need non-zero value

 

Empty Blocks Should Emit Something or be Removed

Issue

There are function with empty blocks. These should do something like emit an event or reverting. If not it should be removed from the contract.

PoC

Total of 5 issues found.

./GolomTrader.sol:459:    fallback() external payable {}
./GolomTrader.sol:461:    receive() external payable {}
./RewardDistributor.sol:313:    fallback() external payable {}
./RewardDistributor.sol:315:    receive() external payable {}
./VoteEscrowCore.sol:604:            try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (

Mitigation

Add emit or revert in the function block.

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

Total of 8 issues found.

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

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2022/04/21/custom-errors/

PoC

Total of 44 issues found.

./VoteEscrowDelegation.sol:72:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
./VoteEscrowDelegation.sol:73:        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
./VoteEscrowDelegation.sol:99:        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
./VoteEscrowDelegation.sol:130:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
./VoteEscrowDelegation.sol:186:        require(blockNumber < block.number, 'VEDelegation: not yet determined');
./VoteEscrowDelegation.sol:211:        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
./VoteEscrowDelegation.sol:239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./GolomTrader.sol:177:        require(signaturesigner == o.signer, 'invalid signature');
./GolomTrader.sol:211-214:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000, 'amt not matching');
./GolomTrader.sol:217:        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
./GolomTrader.sol:222:        require(o.orderType == 0, 'invalid orderType');
./GolomTrader.sol:226:        require(status == 3, 'order not valid');
./GolomTrader.sol:227:        require(amountRemaining >= amount, 'order already filled');
./GolomTrader.sol:235:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:299:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:359:            require(amount == 1, 'only 1 erc721 at 1 time');
./GolomTrader.sol:455:        require(distributorEnableDate <= block.timestamp, 'not allowed');
./RewardDistributor.sol:173:        require(address(ve) != address(0), ' VE not added yet');
./RewardDistributor.sol:181:            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
./RewardDistributor.sol:184:                require(epochs[index] < epoch, 'cant claim for future epochs');
./RewardDistributor.sol:185:                require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
./RewardDistributor.sol:220:        require(address(ve) != address(0), ' VE not added yet');
./RewardDistributor.sol:292:        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
./RewardDistributor.sol:309:        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
./GolomToken.sol:24:        require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
./GolomToken.sol:43:        require(!isAirdropMinted, 'already minted');
./GolomToken.sol:51:        require(!isGenesisRewardMinted, 'already minted');
./GolomToken.sol:69:            require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');
./VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./VoteEscrowCore.sol:894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
./VoteEscrowCore.sol:928:        require(_locked.amount > 0, 'No existing lock found');
./VoteEscrowCore.sol:929:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
./VoteEscrowCore.sol:945:        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
./VoteEscrowCore.sol:946:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
./VoteEscrowCore.sol:982:        require(_locked.amount > 0, 'No existing lock found');
./VoteEscrowCore.sol:983:        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
./VoteEscrowCore.sol:996:        require(_locked.end > block.timestamp, 'Lock expired');
./VoteEscrowCore.sol:997:        require(_locked.amount > 0, 'Nothing is locked');
./VoteEscrowCore.sol:998:        require(unlock_time > _locked.end, 'Can only increase lock duration');
./VoteEscrowCore.sol:999:        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');
./VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
./VoteEscrowCore.sol:1011:        require(block.timestamp >= _locked.end, "The lock didn't expire");
./VoteEscrowCore.sol:1082:        require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');
./VoteEscrowCore.sol:1227:        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

Mitigation

I suggest implementing custom errors to save gas.