Platform: Code4rena
Start Date: 18/10/2022
Pot Size: $50,000 USDC
Total HM: 13
Participants: 67
Period: 5 days
Judge: Picodes
Total Solo HM: 7
Id: 172
League: ETH
Rank: 37/67
Findings: 1
Award: $37.88
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: berndartmueller
Also found by: 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, Aymen0909, BClabs, Diana, Jeiwan, Lambda, LeoS, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Rolezn, SaharAP, Trust, V_B, __141345__, a12jmx, bharg4v, brgltd, carlitox477, ch0bu, chaduke, cloudjunky, cryptostellar5, cryptphi, csanuragjain, d3e4, delfin454000, erictee, fatherOfBlocks, hansfriese, ignacio, joestakey, karanctf, ladboy233, lukris02, mcwildy, minhtrng, peanuts, ret2basic, seyni, slowmoses, svskaushik, tnevler, yixxas
37.8829 USDC - $37.88
| Issue | Instances | |
|---|---|---|
| LOW‑1 | Missing Checks for Address(0x0) | 6 |
| LOW‑2 | Use _safeMint instead of _mint | 4 |
| LOW‑3 | Contracts are not using their OZ Upgradeable counterparts | 4 |
| LOW‑4 | Critical Changes Should Use Two-step Procedure | 6 |
| LOW‑5 | Upgrade OpenZeppelin Contract Dependency | 1 |
Total: 21 instances over 5 issues
| Issue | Instances | |
|---|---|---|
| NC‑1 | Adding A Return Statement When The Function Defines A Named Return Variable, Is Redundant | 1 |
| NC‑2 | Public Functions Not Called By The Contract Should Be Declared External Instead | 4 |
| NC‑3 | require() / revert() Statements Should Have Descriptive Reason Strings | 2 |
| NC‑4 | Implementation contract may not be initialized | 3 |
| NC‑5 | Use of Block.Timestamp | 1 |
| NC‑6 | Non-usage of specific imports | 22 |
| NC‑7 | Lines are too long | 22 |
| NC‑8 | Use bytes.concat() | 2 |
Total: 57 instances over 8 issues
Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.
370: function setDefaultReservedTokenBeneficiary: address _beneficiary
480: function mintFor: address _beneficiary
71: function launchProjectFor: address _owner
854: function recordSetDefaultReservedTokenBeneficiary: address _beneficiary
1123: function recordSetFirstOwnerOf: address _owner
1173: function cleanTiers: address _nft
Consider adding explicit zero-address validation on input parameters of address type.
_safeMint instead of _mintAccording to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-
461: _mint(_reservedTokenBeneficiary, _tokenId);
504: _mint(_beneficiary, _tokenId);
635: _mint(_beneficiary, _tokenId);
677: _mint(_beneficiary, _tokenId);
Use _safeMint whenever possible instead of _mint
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
4: import '@openzeppelin/contracts/utils/Checkpoints.sol';
5: import '@openzeppelin/contracts/access/Ownable.sol';
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
147: function setTierDelegates(JBTiered721SetTierDelegatesData[] memory _setTierDelegatesData)
177: function setTierDelegate(address _delegatee, uint256 _tierId) public virtual override {
370: function setDefaultReservedTokenBeneficiary(address _beneficiary) external override onlyOwner {
386: function setBaseUri(string memory _baseUri) external override onlyOwner {
402: function setContractUri(string calldata _contractUri) external override onlyOwner {
418: function setTokenUriResolver(IJBTokenUriResolver _tokenUriResolver) external override onlyOwner {
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).
@openzeppelin/contracts: ^4.6.0
Update OpenZeppelin Contracts Usage in package.json
631: return leftoverAmount
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
function setTierDelegates(JBTiered721SetTierDelegatesData[] memory _setTierDelegatesData) public virtual override {
function setTierDelegate(address _delegatee, uint256 _tierId) public virtual override {
function mintReservesFor(uint256 _tierId, uint256 _count) public override {
function mintFor(uint16[] memory _tierIds, address _beneficiary) public override onlyOwner returns (uint256[] memory tokenIds) {
require() / revert() Statements Should Have Descriptive Reason Strings216: require(address(this) != codeOrigin);
218: require(address(store) == address(0));
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
constructor() { codeOrigin = address(this); }
constructor( JB721GlobalGovernance _globalGovernance, JB721TieredGovernance _tieredGovernance, JBTiered721Delegate _noGovernance ) { globalGovernance = _globalGovernance; tieredGovernance = _tieredGovernance; noGovernance = _noGovernance; }
constructor( IJBController _controller, IJBTiered721DelegateDeployer _delegateDeployer, IJBOperatorStore _operatorStore ) JBOperatable(_operatorStore) { controller = _controller; delegateDeployer = _delegateDeployer; }
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116
903: if (_storedTierOf[msg.sender][_tierId].lockedUntil >= block.timestamp) revert TIER_LOCKED();
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
4: import './abstract/Votes.sol';
5: import './JBTiered721Delegate.sol';
5: import './interfaces/IJB721TieredGovernance.sol';
6: import './JBTiered721Delegate.sol';
6: import './abstract/JB721Delegate.sol';
7: import './interfaces/IJBTiered721Delegate.sol';
8: import './libraries/JBIpfsDecoder.sol';
9: import './libraries/JBTiered721FundingCycleMetadataResolver.sol';
10: import './structs/JBTiered721Flags.sol';
4: import './interfaces/IJBTiered721DelegateDeployer.sol';
5: import './JBTiered721Delegate.sol';
6: import './JB721TieredGovernance.sol';
7: import './JB721GlobalGovernance.sol';
7: import './interfaces/IJBTiered721DelegateProjectDeployer.sol';
5: import './interfaces/IJBTiered721DelegateStore.sol';
6: import './libraries/JBBitmap.sol';
7: import './structs/JBBitmapWord.sol';
8: import './structs/JBStored721Tier.sol';
10: import '../interfaces/IJB721Delegate.sol';
11: import './ERC721.sol';
4: import '../structs/JBBitmapWord.sol';
4: import './../structs/JBTiered721FundingCycleMetadata.sol';
Use specific imports syntax per solidity docs recommendation.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
539: // Set the leftover amount as the initial value, including any credits the beneficiary might already have.
542: // Keep a reference to a flag indicating if a mint is expected from discretionary funds. Defaults to false, meaning to mint is not expected.
545: // Keep a reference to the flag indicating if the transaction should revert if all provded funds aren't spent. Defaults to false, meaning only a minimum payment is enforced.
548: // Skip the first 32 bytes which are used by the JB protocol to pass the paying project's ID when paying from a JBSplit.
743: // If there's no stored first owner, and the transfer isn't originating from the zero address as expected for mints, store the first owner.
120: // Get a bit of freemem to land the bytecode, not updated as we'll leave this scope right after create(..)
663: // Make sure the tier's contribution floor is greater than or equal to the previous contribution floor.
739: // If this is the last tier being added, track the current last sort index if it's not already tracked.
747: // Set the tier after the previous one being iterated on as the tier being added, or 0 if the index is incremented.
1193: // If the current index being iterated on isn't an increment of the previous, set the correct tier after if needed.
1197: // Otherwise if the current index is an increment of the previous and the after index isn't 0, set it to 0.
1242: // Get a reference to the number of tokens already minted in the tier, not counting reserves or burned tokens.
1250: // Get the number of reserved tokens mintable given the number of non reserved tokens minted. This will round down.
118: // Check the 4 bytes interfaceId and handle the case where the metadata was not intended for this contract
257: // Check the 4 bytes interfaceId and handle the case where the metadata was not intended for this contract
144: // These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.
229: // Make sure the caller is a terminal of the project, and the call is being made on behalf of an interaction with the correct project.
250: // Make sure the caller is a terminal of the project, and the call is being made on behalf of an interaction with the correct project.
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
28: bytes memory completeHexString = abi.encodePacked(bytes2(0x1220)
34: return string(abi.encodePacked(_baseUri, ipfsHash)
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
#0 - Picodes
2022-11-08T16:47:09Z
Some invalid stuffs (contracts are not upgradeable, block.timestamp)
#1 - c4-judge
2022-11-08T16:47:15Z
Picodes marked the issue as grade-b