BIDSHOP

CertiK5 Jun 2023Files named in the report

Audit

CertiK reviewed BIDSHOP on 5 Jun 2023: 37 findings, 0 critical; 33 marked fixed, 2 acknowledged, 2 open.

No open critical findings

Open the report Open BIDSHOP’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings37

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	1	1
Medium	0	1	0
Low	2	0	32

HighIncorrect Calculations In `tryExtendRound()`Fixed

There are 3 possible types of extensions, depending on the total number of chunks and the stored `extenstionStep`: - base type are for when the number of chunks added are less than or equal to `extensionStep` (roundExtensionChunk added for each chunk) - type 1 are for when the number of chunks added are greater than extensionStep and less than or equal to extensionStep*2 (1/2 roundExtensionChunk added for each chunk) - type 2 are for when the number of chunks added exceed extensionStep*2 (1/6 round extensionChunk added for each chunk) The calculation for `helperDelta` in the case where the total chunks exceed `extensionStep * 2` and the `extraChunks` include chunks of base type is incorrect. ```soldity helperDelta = (extraChunks + baseExtensionsHad) - helperDoubleDelta - extensionStep ``` Plugging in the `helperDoubleDelta` and simplifying this equation is equivalent to ```solidity helperDelta = (extraChunks + 2*baseExtensionsHad - 2*extensionStep) ``` Which is the number of type 2 chunks plus the `baseExtensionsHad`, when it should only be the number of type 2 chunks. In addition, in this case the `roundExtension` is not incremented and instead is set to the value. Thus the `roundExtensionChunk` for the `baseExtensionsHad` will not be accounted for.

Low`nonReentrant` On Internal FunctionsFixed

The `nonReentrant` modifier is only used on the internal conduct bid functions. This is not optimal, as it will be performed multiple times when bidding with multiple tokens. Instead the modifier can be added to all user facing external functions. While this will slightly increase the deployment size, it will save gas costs when bidding with multiple tokens.

Low`TODO` CommentsFixed

The linked lines have a `todo` comment.

LowCan Check If `nodeExists` And Only `remove` In ThFixed

In the function `forceWithdraw()` the following code is not optimal: ```solidity if (!expiryBackLog.nodeExists(_user)) expiryBackLog.pushBack(_user); expiryBackLog.remove(_user); ``` In this case if a node does not exist in the backlog for the `_user`, then one is created to then be removed. Instead it can be checked if a node exists in the backlog for the `_user` and only remove it in this case.

LowCan Reduce Contract SizeFixed

The function `conductBidSlimWithVerifier()` has the same logic as `conductBidSlim()` with one additional line at the end which calls the `bidVerifierContract`. Instead of calling `conductBidSlimWithVerifier()`, functions that call this can instead call `conductBidSlim()` and then make the call to the `bidVerifierContract`, allowing the `conductBidSlimWithVerifier()` function to be deleted.

LowContracts Do Not Inherit Their InterfacesFixed

Contracts often inherit their own interfaces to ensure that they have proper implementations. This helps ensure that as changes are made to the codebase that the interfaces will still be compatible.

LowCostly Operation Inside LoopFixed

Accessing storage variables in a loop can be costly in terms of gas consumption. ```solidity=359 cumulativeRanksRatio += _ranksRatio[i]; ``` --- ```solidity=84 vested += _totalWithdrawable[i][lastMonthIndex]; ```

LowDiscussion On `_handleDeservingVolume()`Fixed

If `_delta < _amount` then the `deservingVolume` is set to `_delta`. However, the comment above states that "_delta is greater then amount, thus we penalize", which is the opposite of this logic.

LowDivide Before MultiplyFixed

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. In the function `pendingTokenSplit()` if `pendingBaseRewardsHigh` is nonzero, then `pendingFactorRewards` will always return zero. This is because the following calculation ```solidity pendingFactorRewards = pendingRewards * (pendingFactorRewardsHigh / (pendingBaseRewardsHigh + pendingFactorRewardsHigh)); ``` Will first evaluation the division ```solidity (pendingFactorRewardsHigh / (pendingBaseRewardsHigh + pendingFactorRewardsHigh)) ``` Which will round down to zero.

LowDoes Not Revoke Previous AuthorityFixed

The function `setOnBidMinter()`, changes the `onBidMinter` to be the input `_minter` and sets them as `isMinter`. However, the previous `onBidMinter` will still have the ability to mint as the mapping `isMinter` is never set to false for them.

Show 10 more

LowIf Burned Amount Of Tokens Becomes Large Enough TFixed

In solidity versions `0.8.0` and greater, underflow and overflow are checked automatically. If the `totalBurnedAmount` reaches the maximum `uint256`, then any transfer that would burn tokens will revert due to an overflow error.

LowIncompatibility with Deflationary TokensFixed

When transferring deflationary ERC20 tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user sends 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived to the contract. However, a failure to discount such fees may allow the same user to withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. We give this finding an informational severity as we assume the tokens interacted with below are not intended to be deflationary tokens. However, we want to ensure that deflationary tokens will not be used in any of the scenarios below. ```solidity=264 pool.lpToken.transferFrom(msg.sender, address(this), _amount); ``` - Transferring tokens by `_amount`. ```solidity=261 _updateUserAmount(_pid, _user, user.amount + _amount); ``` - The `_amount` appears to be used for bookkeeping purposes without compensating the potential transfer fees. --- ```solidity=285 pool.lpToken.transferFrom(address(msg.sender), address(this), _amount); ``` - Transferring tokens by `_amount`. ```solidity=282 (reward, additionalRewards) = _updateUserAmount(_pid, msg.sender, user.amount + _amount); ``` - The `_amount` appears to be used for bookkeeping purposes without compensating the potential transfer fees. --- ```solidity=183 bids.safeTransferFrom(msg.sender, address(this), amount); ``` - Transferring tokens by `amount`. ```solidity=180 users[msg.sender].userTotalBidsLocked += amount; ``` - The `amount` appears to be used for bookkeeping purposes without compensating the potential transfer fees.

LowInefficient Memory ParameterFixed

One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. ```solidity=46 function notifyUsersRules( ``` `notifyUsersRules` has memory location parameters: `_users`, `_totalVeBids`, `_totalPseudoLP`, `_totalWithdrawable`.

LowMissing ChecksFixed

In the contract `AuctionBonus`: - In the function `setMinter`, it is not checked if the input `_minter` already has the input `_status`. In the contract `PoolRankedLogicSlim`: - In the function `reveal()` it is not checked that the input arrays `_nonces`, `_bidIds`, and `_bidAmounts` have the same length. - In the function `setRanks()` it is not checked that the input arrays `ranks` and `_ranksRatio` have the same length. In the contract `BidRouterVerifier`: - In the function `bidMultipleTokens()`, `bidMultipleTokensSlim()`, and `bidMultipleTokensSlimWithVerifier()` it should be checked that the input arrays `_tokens`, `_bidsPerToken`, `_ciphers`, and `_hashes` have the same length. In the contract `PoolBaseLogicSlim`: - In the modifier `basePoolInitialization()` it is not checked that the input `roundDuration` is nonzero. If it is set to zero, then there is possible division by zero.

LowMissing Emit EventsFixed

The functions whose lines are provided above do not emit events when changing state variables.

LowMissing Functionality From DocumentationFixed

In your documentation you state: - If “coolOffPeriodTime” isn’t set to 0 during deployment, every 3 `dead` rounds (not reach min bids) there is a period in which users can revoke bids and refund their credits/bonus. - If admin calls the irreversible “killPool” function, bids can no longer be placed. Users can revoke bids from round that are not finalized. While there are cool off periods implemented after every 3 `dead` rounds, there is no functionality allowing users to revoke bids and refund their credits/bonus.

LowMissing Parameter In Natspec CommentsFixed

In the contract `BidRouterVerifier`: - The `natspec` comments for the function `removeFactory()` is missing the parameter `factoryId`. In the contract `AuctionFactoryUniversal`: - The `natspec` comments for the function `addPool()` is missing the parameter `_type`. IN the contract `MasterBids`: - The `natspec` comments for the function `add()` is missing the parameter `_rewarder`.

LowMissing Zero Address ValidationFixed

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=170 onBidMinter = _minter; ``` - `_minter` is not zero-checked before being used. --- ```solidity=47 bidRouter = _bidRouter; ``` - `_bidRouter` is not zero-checked before being used. --- ```solidity=279 bidRouter = _newRouter; ``` - `_newRouter` is not zero-checked before being used. --- ```solidity=87 teamAddress = _teamAddress; ``` ```solidity=88 gasReceiver = _teamAddress; ``` - `_teamAddress` is not zero-checked before being used. --- ```solidity=588 teamAddress = _teamAddress; ``` - `_teamAddress` is not zero-checked before being used. --- ```solidity=595 gasReceiver = _gasReceiver; ``` - `_gasReceiver` is not zero-checked before being used. --- ```solidity=122 voter = _voter; ``` - `_voter` is not zero-checked before being used. --- ```solidity=467 voter = _newVoter; ``` - `_newVoter` is not zero-checked before being used. --- ```solidity=104 feeToken = _feeToken; ``` - `_feeToken` is not zero-checked before being used. --- ```solidity=105 creditToken = _creditToken; ``` - `_creditToken` is not zero-checked before being used. --- ```solidity=106 bonusToken = _bonusToken; ``` - `_bonusToken` is not zero-checked before being used. --- ```solidity=107 bidRouter = _bidRouter; ``` - `_bidRouter` is not zero-checked before being used. --- ```solidity=216 stakingTreasury = _treasury; ``` - `_treasury` is not zero-checked before being used. --- ```solidity=228 bonusToken = _bonusToken; ``` - `_bonusToken` is not zero-checked before being used. --- ```solidity=234 bidRouter = _newRouter; ``` - `_newRouter` is not zero-checked before being used.

LowNo Lower Limit On `expiryDuration`Fixed

The function `changeExpiryDuration()` does not enforce any lower limit on the `expiryDuration`. In the extreme case, this allows the `expiryDuration` to be set to 0, which will cause all tokens to be expired as soon as they are minted causing them to be burned.

LowNo User Should Be Added Twice For A TreeFixed

If a user is added to a tree twice, then the user may not be able to claim both of their airdrops. If a user is accidentally added to be able to claim the amounts `200` and `100` and the user claims the `200` amount first, then the amount of `100` will not be able to be claimed. This is because the `amountClaimedByUserPerTreeId` will be `200` after the first claim so that when the `adjustedAmount` is calculated in `_canClaim()` it will revert due to underflow. We understand that this may be the intended design, which is why we mark the severity as informational. However, we want to ensure that checks are done off chain when constructing the Merkle trees or in the smart contract code, to ensure that a user is only given a single airdrop per Merkle tree.

Show 10 more

LowNon-descriptive Error MessageFixed

In the function `getMonth()`, if the `_blockTimestamp` is less than the `_genesisStamp`, then it will revert due to underflow. If a user attempts to `release()` before the `_genesisStamp`, then they will encounter an underflow error as the cause of the revert, which may cause confusion.

LowNonStandard ERC20 ImplementationFixed

The contracts `AuctionBonus` and `AuctionCredit` both inherit `ERC20`, however they are not designed to implement all functionality and in particular there is no need for allowances. Furthermore, both contracts have `ERC20` functions that they deprecate that simply `revert` if called. It will not return a descriptive error message explaining that the cause of the revert is due to it being a nonstandard ERC20 token and that function being deprecated for it.

LowPossible Collision Of `bidListId` and `bidListSloFixed

In the function `emergencyRevealReset()`, the data re-indexer is determined by the hash of the `block.number`: ```solidity bidListSlotsDataReindexer[_bidListId] = uint256(keccak256(abi.encodePacked(block.number))); ``` However unlikely, it is possible that this operation returns a `uint256` of an existing `_bidListId`, which will then cause the finalization process to overwrite information that should have been stored for the `_bidListId`.

LowPotential Issue With `emergencyWithdraw()`Fixed

The function `emergencyWithdraw()` is designed to be called in an emergency to allow a user to withdraw their `lpToken`, but sacrifice their rewards. This is called in the case the contract does not have enough reward tokens which would cause the normal `withdraw()` function to revert. However, if the `rewarder()` encounters a similar scenario where it will revert when `onReward()` is called, then `emergencyWithdraw()` will revert and not be able to be used in an emergency.

LowPotential Issues With `setMintingAllowance`Fixed

If `setMintingAllowance()` is called on a `_promoter` that has an unused allowance amount before their current `promotionMinterExpiry`, then they can potentially use their entire allowance just before the new allowance is set. If when setting the new allowance, the old allowance was included in the input `amount`, then this would allow the `promoter` to gain more allowance than was intended.

LowPotential ReentrancyFixed

In the contract `AuctionCredit`, reentrancy is possible if the `pegToken` implements hooks which make an external call allowing for reentrancy. While there are comments stating only trusted tokens like WETH will be used, if a token is upgradeable it could potentially be upgraded to allow hooks in the future. We mark this finding with an informational severity to ensure that proper precautions are taken for any potential `pegToken`.

LowTypos and InconsistenciesFixed

In the contract `AuctionBonus`, the following typos and inconsistencies were found: - The comment above the mapping `isMinter` has "@notice" and defines another mapping `mintAllowance` as opposed to describing that this mapping is used to specify addresses that are allowed to mint. - "super.beforeTokenTransfer()" is commented out in the function `_beforeTokenTransfer()`. We recommend removing this comment or rewriting it to describe that you are overriding an empty function to implement restrictions before tokens are transferred. - An error message in `_beforeTokenTransfer()` says "Transfers from user can only to pool", should be "Transfers from user can only be to pool". - The comment above the function `transferFrom()`, "use" is used instead of "user". - The comment above the function `_updateExpiryDate()` states "Expiry resets when biding or receiving minted tokens", however, expiry also resets when bonus bids are revoked. We recommend updating the comment to reflect all instances when expiries will be reset. - The comment above the function `mint()`, states "Miters mints for user" which should be "Minters mint for user". - The comment above `balanceOf()` uses "he's" instead of "his". In the contract `AuctionCredit`, the following typos and inconsistencies were found: - Both `AuctionBonus` and `AuctionCredit` have an expiry, however, they differ on if it is expired when the `block.timestamp` equals the `userExpiry`. In `AuctionBonus` it would be considered expired, however, in `AuctionCredit` it would not be expired. We recommend choosing one of these conventions and adopting it in both contracts to remain consistent. - The comment above the function `_mint()` states "During mint, expired tokens are burn", which should be "During mint, expired tokens are burnt". - The comment above the function `deposit()` uses `warps` instead of `wraps`. - This comment in the function `deposit()` "// pegToken.safeTransferFrom(msg.sender, address(this), _amount);" seems to be outdated code that should be removed. - The comment above the function `promoterMint()` states "Addresses with allowance may provide 1:1 feeToken to mint tokens", however, this should say `pegToken`. In addition, the line of code that would transfer the `pegToken` is commented out. We recommend updating the comment or code to be conistent. - The comments above the function `forceWithdraw()` do not match its functionality as it cannot be called on pools. - A comment in the function `forceWithdraw()` states "1:1 ratio with pegToken, returning pegToken to team", however, the `pegToken` is being returned to the `_user`. In the contract `BidRouterVerifier`, the following typos and inconsistencies were found: - Above the mapping `userPointsThreshold`, "+>" is used instead of "=>". - The comment above the function `setTeamAddress()` states they will receive the gas covering fee, when the gas covering fee is not sent to this address, but to the `gasReceiver` address. In the contract `VeERC20`, the following typos and inconsistencies were found: - The mapping `_allowances` is defined, however, it is never used and the comments state that allowances are disabled. In the contract `PoolBaseLogicSlim`, the following typos and inconsistencies were found: - The comment above the modifier `poolAlive` has "nftsSh" instead of "nfts". - The comment above the modifier `baseBidFeesEnforcement` states "enforces the facevalue * 2 is bidFee * minBids", however it "enforces bidFee * minBids is at least the faceValue". In the contract `PoolRankedLogicSlim`, the following typos and inconsistencies were found: - The comments in the function `getRoundStatus()`, include commented out code that should be removed. - The comments in the function `reveal()`, include commented out code that should be removed. - There are comments and error messages that include another language than english. As all other comments and errors are in english, we recommend writing them in english as well to remain consistent. In the contract `VeBids`, the following typos and inconsistencies were found: - The comment in the struct `UserInfo` is unnecessary and can be deleted. In the contract `MasterBids`, the following typos and inconsistencies were found: - The bad attempt and commented out code in the function `pendingTokensSplit()` should be deleted.

LowUnchecked Blocks Can Optimize ContractFixed

In general, the counter in a for loop can be incremented or decremented in an unchecked block. In the contract `VeERC20`: - In the function `_mint()`, the increase to the account balance can be placed in an unchecked block. This is because the balances can never exceed the total supply, which is checked above it. - In the function `_burn()`, the decrease to the total supply can be placed in an unchecked block. This is because the amount is less than or equal to the account balance which must be less than or equal to the total supply.

LowUnchecked ERC-20 `transfer()`/`transferFrom()` CaFixed

The return value of the transfer()/transferFrom() call is not checked. ```solidity=264 pool.lpToken.transferFrom(msg.sender, address(this), _amount); ``` --- ```solidity=285 pool.lpToken.transferFrom(address(msg.sender), address(this), _amount); ``` --- ```solidity=349 Bids.transfer(payable(msg.sender), reward); ``` --- ```solidity=369 pool.lpToken.transfer(msg.sender, _amount); ``` --- ```solidity=392 Bids.transfer(payable(_user), reward); ``` --- ```solidity=425 pool.lpToken.transfer(address(msg.sender), user.amount); ``` --- ```solidity=515 Bids.transfer(address(msg.sender), Bids.balanceOf(address(this))); ``` --- ```solidity=205 bids.transfer(msg.sender, breeding.bidsAmount); ```

LowUnused EventFixed

In the contract `AuctionBonus`, the event `MintAllowanceSet` is defined, but never used. We recommend either implementing a mint allowance and emitting the event in the appropriate function or removing the event.

Show 3 more

LowUsage of `transfer()`/`send()` for sending EtherFixed

Using Solidity's `transfer()` and `send()` functions for transferring Ether is not recommended, since some contracts may not be able to receive the funds. These functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Additionally, gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation.

LowVariables That Could Be Declared as ImmutableFixed

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.

LowWill not support fee tokens with more than 18 decFixed

This contract is not compatible with `feeToken` that will use more than 18 decimal places. This is because of the `factoryTokenDecimalDelta` calculation, that will revert due to underflow in this case.