Platform: Code4rena
Start Date: 26/07/2022
Pot Size: $75,000 USDC
Total HM: 29
Participants: 179
Period: 6 days
Judge: LSDan
Total Solo HM: 6
Id: 148
League: ETH
Rank: 106/179
Findings: 2
Award: $56.49
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0x52, 0xA5DF, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xc0ffEE, 0xf15ers, 0xmatt, 0xsanson, 0xsolstars, 8olidity, AuditsAreUS, Bahurum, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chom, CryptoMartian, Deivitto, DevABDee, Dravee, ElKu, Franfran, Funen, GalloDaSballo, GimelSec, GiveMeTestEther, Green, JC, Jmaxmanblue, JohnSmith, Jujic, Junnon, Kenshin, Krow10, Kumpa, Lambda, MEP, Maxime, MiloTruck, Mohandes, NoamYakov, Picodes, RedOneN, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, Soosh, StErMi, StyxRave, Tadashi, TomJ, Treasure-Seeker, TrungOre, Waze, _Adam, __141345__, ajtra, ak1, apostle0x01, arcoun, asutorufos, async, benbaessler, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, carlitox477, chatch, codetilda, codexploder, cryptonue, cryptphi, csanuragjain, cthulhu_cult, delfin454000, dipp, dirk_y, djxploit, ellahi, exd0tpy, fatherOfBlocks, giovannidisiena, hansfriese, horsefacts, hyh, idkwhatimdoing, indijanc, jayfromthe13th, jayphbee, joestakey, kenzo, kyteg, lucacez, luckypanda, mics, minhquanym, obront, oyc_109, pedr02b2, rajatbeladiya, rbserver, reassor, robee, rokinot, rotcivegaf, sach1r0, saian, saneryee, sashik_eth, scaraven, shenwilly, simon135, sseefried, supernova, teddav, ych18, zuhaibmohd, zzzitron
35.1687 USDC - $35.17
@param statements/// @dev function to fill a signed order of ordertype 2 also has a payment param in case the taker wants /// 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 /// @param o the Order struct to be filled must be orderType 2 /// @param amount the amount of times the order is to be filled(useful for ERC1155) /// @param referrer referrer of the order /// @param p any extra payment that the taker of this order wanna send on succesful execution of order function fillCriteriaBid( Order calldata o, uint256 amount, uint256 tokenId, bytes32[] calldata proof, address referrer, Payment calldata p ) public nonReentrant {
@param statements missing for tokenId and proof
/// @notice Deposit `_value` additional tokens for `_tokenId` without modifying the unlock time /// @param _value Amount of tokens to deposit and add to the lock function increase_amount(uint256 _tokenId, uint256 _value) external nonreentrant {
@param statement missing for _tokenId
/// @notice Extend the unlock time for `_tokenId` /// @param _lock_duration New number of seconds until tokens unlock function increase_unlock_time(uint256 _tokenId, uint256 _lock_duration) external nonreentrant {
@param statement missing for _tokenId
@dev commentThe first function below returns number of NFTs whereas the second function returns balance, however @dev is identical for both
/// @dev Returns the number of NFTs owned by `_owner`. /// Throws if `_owner` is the zero address. NFTs assigned to the zero address are considered invalid. /// @param _owner Address for whom to query the balance. function _balance(address _owner) internal view returns (uint256) { return ownerToNFTokenCount[_owner]; } /// @dev Returns the number of NFTs owned by `_owner`. /// Throws if `_owner` is the zero address. NFTs assigned to the zero address are considered invalid. /// @param _owner Address for whom to query the balance. function balanceOf(address _owner) external view returns (uint256) { return _balance(_owner); }
Recommendation: Modify the second @dev comment
require revert stringRequire message should be included to enable users to understand reason for failure
Recommendation: Add a brief (<= 32 char) explanatory message to each of the 31 requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).
require(msg.sender == trader);
Both lines referenced below contain the same require statement:
require(_isApprovedOrOwner(_sender, _tokenId));
Both lines referenced below contain the same require statement:
require(epochs[index] < epoch);
All three lines referenced below contain the same require statement:
require(msg.sender == o.reservedAddress);
require( o.totalAmt * amount > (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller
require(o.orderType == 1);
Both lines referenced below contain the same require statement:
require(status == 3);
Both lines referenced below contain the same require statement:
require(amountRemaining >= amount);
require(o.signer == msg.sender);
require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
require(o.orderType == 2);
require(_entered_state == _not_entered);
require(owner != address(0));
require(_approved != owner);
require(senderIsOwner || senderIsApprovedForAll);
All five lines referenced below contain the same require statement:
require(msg.sender == voter);
require(_from != _to);
require(_isApprovedOrOwner(msg.sender, _from));
require(_isApprovedOrOwner(msg.sender, _to));
Both lines referenced below contain the same require statement:
require(_value > 0); // dev: need non-zero value
Require message is inadequateA revert string should provide enough information for users to understand reason for failure
require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
Change mgmtm to a more complete (but still brief <= 32 char) error message
// Additionally mint function is called to mint the tokens, only the reward distributor contract will be able to mint the token
Change Additionally to Additional
/// @notice sets the minter with timelock, once setup admin needs to call executeSetMinter()
Change setup admin to set up, admin
The same typo (Exeute) occurs in both lines referenced below:
VoteEscrowDelegation.sol: L227
/// @dev Exeute transfer of a NFT.
Change Exeute to Execute in both cases
The same typo (epoc) occurs in all four lines referenced below:
Example (RewardDistributor.sol: L61):
mapping(uint256 => uint256) public rewardTrader; // reward minted each epoc for trader
Change epoc to epoch in each case
/// @dev Add fees contributed by the Seller of nft and the exchange/frontend that facilated the trade
Change facilated to facilitated
The same typo (then) occurs in both lines referenced below:
Example (RewardDistributor.sol: L140):
// if supply is greater then a billion dont mint anything, dont add trades
Change then to than in both cases
// this assumes atleast 1 trade is done daily??????
Change atleast to at least
// emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining
Change both begiining and begining to beginning
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.
Remove repeated word rewards at the end of the comment
The same typo (there) occurs in both lines referenced below:
Example (RewardDistributor.sol: L140):
// allows sellers of nft to claim there previous epoch rewards
Change there to their in both cases
// allows exchange that facilated the nft trades to claim there previous epoch rewards
Change facilated to facilitated and there to their
/// @notice Execute's the change trader function
Change Execute's to Executes
/// OrderStatus = 1 , if deadline has been
Change been to past
/// @dev function to settle balances when a bid is filled succesfully
Change succesfully to successfully
The same typo (succesful) occurs in all five lines referenced below:
Example (GolomTrader.sol: L53):
Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
Change succesful to successful in each case
Payment prePayment; // another payment , can be used for royalty, facilating trades
Change facilating to facilitating
uint256 nonce; // nonce of order usefull for cancelling in bulk
Change usefull to useful
/// @param old_locked Pevious locked amount / end lock time for the user
Change Pevious to Previous
While a comment may contain abbreviations, shorthand for well-known or previously defined terms, inconsistent or nonstandard punctuation, and use of minor grammatical errors, it should be readable. In other words, it should communicate clearly, immediately and without ambiguity. The readability of the set of comments below should be improved:
// at starttime epoch 1 starts , first trade changes epoch from 0 to 1 , emits tokens stores the rewards for epoch 1 , // after 1 day , first trade changes epoch from 1 to 2, changes eth in contract to weth and stores rewardstakedeth , emits tokens stores the rewards for epoch 2
In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable, there are cases where extremely long comments interfere with readability.
Below are five of the longest comments whose readability could be improved, as shown:
/// @dev function to fill a signed order of ordertype 2 also has a payment param in case the taker wants /// 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
Suggestion:
/// @dev function to fill a signed order of ordertype 2 also has a payment param in case /// the taker wants to send ether to that address on filling the order, match a 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.
/// @return bool whether the msg.sender is approved for the given token ID, is an operator of the owner, or is the owner of the token
Suggestion:
/// @return bool whether the msg.sender is approved for the given token ID, /// is an operator of the owner, or is the owner of the token.
/// @dev returns unclaimed rewards of an NFT, returns (unclaimed golom rewards, unclaimed eth rewards, unclaimed epochs)
Suggestion:
/// @dev returns unclaimed rewards of an NFT, /// returns (unclaimed golom rewards, unclaimed eth rewards, unclaimed epochs).
/// @dev Throws unless `msg.sender` is the current owner, an authorized operator, or the approved address for this NFT.
Suggestion:
/// @dev Throws unless `msg.sender` is the current owner, /// an authorized operator, or the approved address for this NFT.
/// @dev Set or reaffirm the approved address for an NFT. The zero address indicates there is no approved address.
Suggestion:
/// @dev Set or reaffirm the approved address for an NFT — /// the zero address indicates there is no approved address.
Comments that contain TODOs or other language indicating open items should be addressed and modified or removed. Below are two instances:
// this assumes atleast 1 trade is done daily??????
// Hopefully it won't happen that this won't get used in 5 years! // If it does, users will be able to withdraw but vote weight will be broken
For readability, scientific notation for large multiples of ten is preferable to using exponentiation
uint256 constant dailyEmission = 600000 * 10**18;
if (rewardToken.totalSupply() > 1000000000 * 10**18) {
Suggestion: Change 10**18 to 1e18 in both cases
🌟 Selected for report: JohnSmith
Also found by: 0x1f8b, 0xA5DF, 0xDjango, 0xKitsune, 0xLovesleep, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Aymen0909, Bnke0x0, CRYP70, Chandr, Chinmay, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, Funen, GalloDaSballo, Green, IllIllI, JC, Jmaxmanblue, Junnon, Kaiziron, Kenshin, Krow10, Maxime, Migue, MiloTruck, Noah3o6, NoamYakov, Randyyy, RedOneN, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, StyxRave, TomJ, Tomio, _Adam, __141345__, ajtra, ak1, apostle0x01, asutorufos, async, benbaessler, brgltd, c3phas, cRat1st0s, carlitox477, delfin454000, djxploit, durianSausage, ellahi, erictee, fatherOfBlocks, gerdusx, gogo, hyh, jayfromthe13th, jayphbee, joestakey, kaden, kenzo, kyteg, ladboy233, lucacez, m_Rassska, mics, minhquanym, oyc_109, pfapostol, rbserver, reassor, rfa, robee, rokinot, sach1r0, saian, samruna, sashik_eth, simon135, supernova, tofunmi, zuhaibmohd
21.3211 USDC - $21.32
require functionSplitting into separate require() functions saves gas
The same require function with embedded && occurs in all three sets of lines referenced below:
VoteEscrowDelegation.sol: L239
require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
Recommendation:
require(attachments[_tokenId] == 0, 'attached'); require(!voted[_tokenId], 'error message');
!= 0 instead of > 0 in a require statement if variable is an unsigned integer!= 0 should be used where possible since > 0 costs more gas
The same require function incorporating > 0 occurs in both lines referenced below:
require(_value > 0); // dev: need non-zero value
Recommendation: Change _value > 0 to _value != 0 in both cases
Similar require functions incorporating _locked.amount > 0 occur in all three lines referenced below:
Example (VoteEscrowCore.sol: L928):
require(_locked.amount > 0, 'No existing lock found');
Recommendation: Change _locked.amount > 0 to _locked.amount != 0 in each case
Require revert string is too longThe revert strings below can be shortened to 32 characters or fewer (as shown) to save gas
require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
Change message to VEDelegation: Need more vote pwr
require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
Change message to Can only claim for sngl Address
require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
Change message to RewardDistributor: time not over
require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
Change message to RewardDistributor: time not over
Both lines referenced below contain the same require:
require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
Change message to Cannot add to exp lock. Withdraw
require(unlock_time > block.timestamp, 'Can only lock until time in the future');
Change message to Can only lock til time in future
The revert string below is also longer than 32 characters but it's not clear how to shorten it
require(msg.sender == minter, 'GolomToken: only reward distributor can enable');
Initializing uint variables to their default value of 0 is unnecessary and costs gas
uint256 public MIN_VOTING_POWER_REQUIRED = 0;
Change to:
uint256 public MIN_VOTING_POWER_REQUIRED;
Similarly for the remaining variables below.
VoteEscrowDelegation.sol: L147
uint256 lower = 0;
votes is initialized to zero in both lines referenced below:
VoteEscrowDelegation.sol: L170
VoteEscrowDelegation.sol: L188
uint256 votes = 0;
uint256 public epoch = 0;
Change to uint256 public epoch;
reward is initialized to zero in all six lines referenced below:
uint256 reward = 0;
rewardEth is initialized to zero in both lines referenced below:
uint256 rewardEth = 0;
uint256 block_slope = 0; // dblock/dt
_min is initialized to zero in both lines referenced below:
uint256 _min = 0;
uint256 d_block = 0;
uint256 d_t = 0;
uint256 dt = 0;
It costs less gas to inline the code of functions that are only called once. Doing so saves the cost of allocating the function selector, and the cost of the jump when the function is called.
modifier onlyMinter() { require(msg.sender == minter, 'GolomToken: only reward distributor can enable'); _; } constructor(address _governance) ERC20('Golom', 'GOL') ERC20Permit('Golom') { _transferOwnership(_governance); // set the new owner } /// @notice Mints the tokens /// @dev only minter can mint the tokens, minter will be RewardDistributor.sol /// @param _account Address where the tokens will be minted /// @param _amount Number of tokens to be minted function mint(address _account, uint256 _amount) external onlyMinter { _mint(_account, _amount); }
Since onlyMinter() is used only once in this contract (in function mint()) as shown above, it should be inlined to save gas
RewardDistributor.sol: L87-103
modifier onlyTrader() { require(msg.sender == trader); _; } // at starttime epoch 1 starts , first trade changes epoch from 0 to 1 , emits tokens stores the rewards for epoch 1 , // after 1 day , first trade changes epoch from 1 to 2, changes eth in contract to weth and stores rewardstakedeth , emits tokens stores the rewards for epoch 2 /// @dev Add fees contributed by the Seller of nft and the exchange/frontend that facilated the trade /// @param addr the address that contributed in fees /// @param fee the fee contributed by these addresses function addFee(address[2] memory addr, uint256 fee) public onlyTrader { //console.log(block.timestamp,epoch,fee); if (rewardToken.totalSupply() > 1000000000 * 10**18) { // if supply is greater then a billion dont mint anything, dont add trades return; }
Similarly, since onlyTrader() is used only once in this contract (in function addFee()) as shown above, it should be inlined to save gas
if when a require function could be usedConverting if with revert to require function can save gas
if (computedHash != root) { revert('invalid proof'); }
Suggestion:
require(computedHash = root, 'invalid proof ');
for loopSince calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop
VoteEscrowDelegation.sol: L171-173
for (uint256 index = 0; index < delegated.length; index++) { votes = votes + this.balanceOfNFT(delegated[index]); }
Suggestion:
uint256 totalDelegatedLength = delegated.length; for (uint256 index= 0; index< totalDelegatedLength; index++) { votes = votes + this.balanceOfNFT(delegated[index]); }
Similarly for the seven for loops referenced below:
VoteEscrowDelegation.sol: L189-191
VoteEscrowDelegation.sol: L199-205
RewardDistributor.sol: L143-150
RewardDistributor.sol: L157-164
RewardDistributor.sol: L180-210
RewardDistributor.sol: L183-206
Note that I will provide an example at the end of this section that combines all the gas-saving recommendations pertaining to for loops
++index (or equivalent counter) instead of index++ to increase count in a for loopSince use of index++ costs more gas, it is better to use ++index
VoteEscrowDelegation.sol: L171-173
for (uint256 index = 0; index < delegated.length; index++) { votes = votes + this.balanceOfNFT(delegated[index]); }
Suggestion:
for (uint256 index = 0; index < delegated.length; ++index) { votes = votes + this.balanceOfNFT(delegated[index]); }
Similarly for the seven for loops referenced below:
VoteEscrowDelegation.sol: L189-191
VoteEscrowDelegation.sol: L199-205
RewardDistributor.sol: L143-150
RewardDistributor.sol: L157-164
RewardDistributor.sol: L180-210
RewardDistributor.sol: L183-206
for loopUnderflow/overflow checks are made every time index++ (or equivalent counter) is called. Such checks are unnecessary since index is already limited. Therefore, use unchecked{++index} instead
VoteEscrowDelegation.sol: L171-173
for (uint256 index = 0; index < delegated.length; index++) { votes = votes + this.balanceOfNFT(delegated[index]); }
Suggestion:
for (uint256 index = 0; index < delegated.length;) { votes = votes + this.balanceOfNFT(delegated[index]); unchecked{ index++; } }
Similarly for the fourteen for loops referenced below:
VoteEscrowDelegation.sol: L189-191
VoteEscrowDelegation.sol: L199-205
RewardDistributor.sol: L143-150
RewardDistributor.sol: L157-164
RewardDistributor.sol: L180-210
RewardDistributor.sol: L183-206
RewardDistributor.sol: L226-247
RewardDistributor.sol: L258-263
RewardDistributor.sol: L273-278
VoteEscrowCore.sol: L1044-1057
VoteEscrowCore.sol: L1115-1126
VoteEscrowCore.sol: L1167-1181
For loop gas optimization exampleWhile, for presentation purposes, I have separated the for loop-related gas savings opportunities above, they should be combined. Below I show how all three of the gas saving methods outlined in this submission can be implemented together.
++index rather than index++unchecked ++iVoteEscrowDelegation.sol: L171-173
for (uint256 index = 0; index < delegated.length; index++) { votes = votes + this.balanceOfNFT(delegated[index]); }
Suggestion:
uint256 totalDelegatedLength = delegated.length; for (uint256 index = 0; index < totalDelegatedLength;) { votes = votes + this.balanceOfNFT(delegated[index]); unchecked{ ++index; } }