# Require error message too long Require error message is over 32 bytes and will cost most more `VEDelegation: Need more voting power`. I suggest making it shorter by changing it to `Need more voting power` ```solidity File: contracts/vote-escrow/VoteEscrowDelegation.sol #73 require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power'); ``` Works also : ``` File: contracts/rewards/RewardDistributor.sol #181 require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together'); ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L73 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L181 # Should use calldata parameters instead of memory Since the `_delegatedTokenIds` is not modified we could use it as an calldata parameter instead of memory inside the `_writeCheckpoint` function. ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #97 function _writeCheckpoint( uint256 toTokenId, uint256 nCheckpoints, uint256[] memory _delegatedTokenIds ) internal { ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L97 # Store storage variable inside memory variable to reduce gas cost The `checkpoints[toTokenId]` is potentially called 3 times within the same `_writeCheckpoint` so I believe we could potentially store it inside a memory variable in order to reduce gas cost every time it is called and then update the variable in storage accordingly. ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #101 Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1]; ``` ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #106 checkpoints[toTokenId][nCheckpoints] ``` ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #107 checkpoints[toTokenId][nCheckpoints] numCheckpoints[toTokenId] = nCheckpoints + 1 ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L101 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L106 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L107 # Optimise for loops In order to optimise for loops, I would advise storing `.length` method's return inside a memory variable. Also we can assume that the iterator `i` will not overflow so it is safe to use the `uncheck` attribute and replace it by `++i` in order to reduce useless opcodes and reduce gas cost. ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #171 for (uint256 index = 0; index < delegated.length; index++) { votes = votes + this.balanceOfNFT(delegated[index]); } ``` could be replaced by ``` uint256 delegatedLength = delegated.length; for (uint256 index = 0; index < delegatedLength;) { votes = votes + this.balanceOfNFT(delegated[index]); unchecked {++i} } ``` Same for: ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #189 for (uint256 index = 0; index < delegatednft.length; index++) { votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber); } ``` could be replaced by ``` uint256 delegatedNftLength = delegatednft.length; for (uint256 index = 0; index < delegatedNftLength;) { votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber); unchecked { index++} } ``` ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #199 for (uint256 i; i < _array.length; i++) { if (_array[i] == _element) { _array[i] = _array[_array.length - 1]; _array.pop(); break; } } ``` could be replaced by ``` uint256 arrayLength = _array.length; for (uint256 i; i < arrayLength;) { if (_array[i] == _element) { _array[i] = _array[arrayLength - 1]; _array.pop(); break; } uncheck { +ii} } ``` ``` File: contracts/rewards/RewardDistributor.sol #156 uint256 reward = 0; for (uint256 index = 0; index < epochs.length; index++) { require(epochs[index] < epoch); reward = reward + (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) / epochTotalFee[epochs[index]]; feesExchange[addr][epochs[index]] = 0; } ``` could be replace by ``` uint256 epochsLength = epochs.length; for (uint256 index = 0; index < epochsLength;) { require(epochs[index] < epoch); reward = reward + (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) / epochTotalFee[epochs[index]]; feesExchange[addr][epochs[index]] = 0; uncheck {++index} } ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L199 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L258 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L273 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L143 # Change order in which way the require is located In order to reduce gas cost for users that will not pass the following require : `require(_isApprovedOrOwner(_sender, _tokenId))` I believe we should move up this require before the `this.removeDelegation(_tokenId);` since this function is pretty costly and has no impact on the next require. ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #245 require(_isApprovedOrOwner(_sender, _tokenId)) ``` should be before ``` File: contracts/vote-escrow/VoteEscrowDelegation.sol #242 this.removeDelegation(_tokenId) ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L245 # Cheaper to not instantiate storage variable by 0 It cheaper to not instantiate variable by 0 and let the EVM automatically set it to 0 ``` File: contracts/rewards/RewardDistributor.sol #45 uint256 public epoch = 0; ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L45 # Repack `Order` struct in order to use less storage slots In order to make the `Order` struct more gas efficient we should rethink the way it is packed. For that we have the move the `uint8 v` next to a 20 bytes address like `address collection` or `reservedAddress` so that instead of respectively take 2 slots they will only use one. The initial ``` File: contracts/core/GolomTrader.sol #47 struct Order { address collection; // NFT contract address 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 bool isERC721; // standard of the collection , if 721 then true , if 1155 then false 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 uint8 v; bytes32 r; bytes32 s; } ``` could be changed to: ``` struct Order { address collection; // NFT contract address uint8 v; uint256 tokenId; // order for which tokenId of the collection address signer; // maker of order address uint256 orderType; uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount Payment exchange; Payment prePayment; // another payment , can be used for royalty, facilating trades bool isERC721; // standard of the collection , if 721 then true , if 1155 then false 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; 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/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L45 # Use `abi.encodePacked` over `abi.encode` It is more efficient to use the `abi.encodePacked` over the `abi.encode` method and should be used in this case since we will then hash the result of this method into a 32 bytes variable using the `keccak256` function ``` File: contracts/core/GolomTrader.sol #102 EIP712_DOMAIN_TYPEHASH = keccak256( abi.encode( keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'), keccak256(bytes('GOLOM.IO')), keccak256(bytes('1')), chainId, address(this) ) ); ``` Same for: ``` File: contracts/core/GolomTrader.sol #114 keccak256( abi.encode( keccak256('payment(uint256 paymentAmt,address paymentAddress)'), p.paymentAmt, p.paymentAddress ) ); ``` ``` File: contracts/core/GolomTrader.sol #129 keccak256( abi.encode( keccak256( 'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)' ), o.collection, o.tokenId, o.signer, o.orderType, o.totalAmt, hashPayment(o.exchange), hashPayment(o.prePayment), o.isERC721, o.tokenAmt, o.refererrAmt, o.root, o.reservedAddress, extra ) ); ``` https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L102 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L114 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L129

Maxime