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: 26/67
Findings: 2
Award: $63.84
🌟 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
In the contracts, floating pragmas should not be used. 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.
Proof of Concept https://swcregistry.io/docs/SWC-103
All contracts have floating pragmas
Check here for NatSpec rules: https://docs.soliditylang.org/en/develop/natspec-format.html
missing @author
missing @author missing @title ... missing @param _targetAddress, @return _out 115 function _clone(address _targetAddress) internal returns (address _out) {
missing @author missing @title
missing @author missing @return 74 function getTierDelegate(address _account, uint256 _tier) missing @return 90 function getTierVotes(address _account, uint256 _tier) external view override returns (uint256) { missing @return 102 function getPastTierVotes( missing @return 116 function getTierTotalVotes(uint256 _tier) external view override returns (uint256) { missing @return 127 function getPastTierTotalVotes(uint256 _tier, uint256 _blockNumber) missing @return 194 function _getTierVotingUnits(address _account, uint256 _tierId) missing @param a, @param b, @ return 322 function _add(uint256 a, uint256 b) internal pure returns (uint256) { missing @param a, @param b, @ return 326 function _subtract(uint256 a, uint256 b) internal pure returns (uint256) {
missing @author missing @return 175 function supportsInterface(bytes4 _interfaceId) public view override returns (bool) {
missing @author
missing @author missing @return 178 function supportsInterface(bytes4 _interfaceId)
missing @author missing @title missing @param _baseUri, @param _hexString, @return 22 function decode(string memory _baseUri, bytes32 _hexString) missing @param _source, @return 44 function _toBase58(bytes memory _source) private pure returns (string memory) { missing @param _array, @param _length, @ return 66 function _truncate(uint8[] memory _array, uint8 _length) private pure returns (uint8[] memory) { missing @param _input, @return 74 function _reverse(uint8[] memory _input) private pure returns (uint8[] memory) { missing @param _indices, @ return 82 function _toAlphabet(uint8[] memory _indices) private pure returns (bytes memory) {
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI).
Missing NatSpec
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so
123 function balanceOf(address _owner) public view override returns (uint256 balance) { 138 function tokenURI(uint256 _tokenId) public view override returns (string memory) { 175 function supportsInterface(bytes4 _interfaceId) public view override returns (bool) {
499 function balanceOf(address _nft, address _owner) public view override returns (uint256 balance) { 523 function redemptionWeightOf(address _nft, uint256[] calldata _tokenIds) 550 function totalRedemptionWeight(address _nft) public view override returns (uint256 weight) {
178 function supportsInterface(bytes4 _interfaceId)
Consider changing the variable to be an unnamed one
37 returns (uint256 units)
72 ) external override returns (IJBTiered721Delegate) {
162 returns (uint256 configuration)
123 function balanceOf(address _owner) public view override returns (uint256 balance) {
Setters should check the input value - ie make revert if it is the zero address or zero
370 function setDefaultReservedTokenBeneficiary(address _beneficiary) external override onlyOwner { 371 // Set the beneficiary. 372 store.recordSetDefaultReservedTokenBeneficiary(_beneficiary);
177 function setTierDelegate(address _delegatee, uint256 _tierId) public virtual override { 178 _delegateTier(msg.sender, _delegatee, _tierId);
854 function recordSetDefaultReservedTokenBeneficiary(address _beneficiary) external override { 855 defaultReservedTokenBeneficiaryOf[msg.sender] = _beneficiary; ... 1123 function recordSetFirstOwnerOf(uint256 _tokenId, address _owner) external override { 1124 firstOwnerOf[msg.sender][_tokenId] = _owner;
As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”
Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”
202 function initialize(
#0 - c4-judge
2022-11-08T17:39:43Z
Picodes marked the issue as grade-b
🌟 Selected for report: Jeiwan
Also found by: 0x1f8b, 0x4non, 0x5rings, 0xSmartContract, Awesome, Aymen0909, Bnke0x0, CodingNameKiki, Diana, DimSon, JC, JrNet, LeoS, RaymondFam, ReyAdmirado, Saintcode_, Shinchan, __141345__, berndartmueller, bharg4v, brgltd, carlitox477, ch0bu, chaduke, cryptostellar5, emrekocak, gogo, lukris02, martin, mcwildy, sakman, trustindistrust, zishansami
25.9629 USDC - $25.96
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
543 bool _expectMintFromExtraFunds; 546 bool _dontOverspend; 616 bool _expectMint
1015 bool _isManualMint
39 return store.votingUnitsOf(address(this), _account);
220 return 221 controller.launchFundingCyclesFor( 222 _projectId, 223 _launchFundingCyclesData.data, 224 _launchFundingCyclesData.metadata, 225 _launchFundingCyclesData.mustStartAtOrAfter, 226 _launchFundingCyclesData.groupedSplits, 227 _launchFundingCyclesData.fundAccessConstraints, 228 _launchFundingCyclesData.terminals, 229 _launchFundingCyclesData.memo 230 ); ... 246 return 247 controller.reconfigureFundingCyclesOf( 248 _projectId, 249 _reconfigureFundingCyclesData.data, 250 _reconfigureFundingCyclesData.metadata, 251 _reconfigureFundingCyclesData.mustStartAtOrAfter, 252 _reconfigureFundingCyclesData.groupedSplits, 253 _reconfigureFundingCyclesData.fundAccessConstraints, 254 _reconfigureFundingCyclesData.memo 255 );
80 return _tierDelegation[_account][_tier]; 91 return _delegateTierCheckpoints[_account][_tier].latest(); 107 return _delegateTierCheckpoints[_account][_tier].getAtBlock(_blockNumber); 117 return _totalTierCheckpoints[_tier].latest(); 134 return _totalTierCheckpoints[_tier].getAtBlock(_blockNumber); 200 return store.tierVotingUnitsOf(address(this), _account, _tierId); 323 return a + b; 327 return a - b;
108 return _owners[_tokenId]; 124 return store.balanceOf(address(this), _owner); ... 149 return 150 JBIpfsDecoder.decode( 151 store.baseUriOf(address(this)), 152 store.encodedTierIPFSUriOf(address(this), _tokenId) 153 ); ... 163 return store.contractUriOf(address(this)); 702 return store.redemptionWeightOf(address(this), _tokenIds); 712 return store.totalRedemptionWeight(address(this));
438 return _balance * _storedTierOf[_nft][_tierId].votingUnits; 456 return encodedIPFSUriOf[_nft][tierIdOfToken(_tokenId)]; 468 return _flagsOf[_nft]; 483 return _bitmapWord.isTierIdRemoved(_tierId); 587 return uint256(uint16(_tokenId)); 1230 if (_storedTier.initialQuantity == 0 || _storedTier.reservedRate == 0) return 0; 1233 if (_storedTier.initialQuantity == _storedTier.remainingQuantity) return 1; 1240 return 0; 1303 return _index + 1;
146 return (_base, _data.memo, delegateAllocations); ... 149 return ( 150 PRBMath.mulDiv( 151 _base, 152 _data.redemptionRate + 153 PRBMath.mulDiv( 154 _redemptionWeight, 155 JBConstants.MAX_REDEMPTION_RATE - _data.redemptionRate, 156 _total 157 ), 158 JBConstants.MAX_REDEMPTION_RATE 159 ), 160 _data.memo, 161 delegateAllocations 162 ); ... 325 return 0; 335 return 0;
22 return JBBitmapWord({currentWord: self[_depth], currentDepth: _depth}); 30 return (self.currentWord >> (_index % 256)) & 1 == 1; 43 return isTierIdRemoved(JBBitmapWord({currentWord: self[_depth], currentDepth: _depth}), _index); 64 return _retrieveDepth(_index) != self.currentDepth; 74 return _index / 256;
34 return string(abi.encodePacked(_baseUri, ipfsHash)); 63 return string(_toAlphabet(_reverse(_truncate(digits, digitlength))));
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so
123 function balanceOf(address _owner) public view override returns (uint256 balance) { 138 function tokenURI(uint256 _tokenId) public view override returns (string memory) { 175 function supportsInterface(bytes4 _interfaceId) public view override returns (bool) {
499 function balanceOf(address _nft, address _owner) public view override returns (uint256 balance) { 523 function redemptionWeightOf(address _nft, uint256[] calldata _tokenIds) 550 function totalRedemptionWeight(address _nft) public view override returns (uint256 weight) {
178 function supportsInterface(bytes4 _interfaceId)
Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.
51 globalGovernance = _globalGovernance; 52 tieredGovernance = _tieredGovernance; 53 noGovernance = _noGovernance;
52 controller = _controller; 53 delegateDeployer = _delegateDeployer;
211 projectId = _projectId; 212 directory = _directory;
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
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 { 480 function mintFor(uint16[] memory _tierIds, address _beneficiary)
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.
44 mapping(address => mapping(uint256 => address)) internal _tierDelegation; 53 mapping(address => mapping(uint256 => Checkpoints.History)) internal _delegateTierCheckpoints;
57 mapping(address => mapping(uint256 => uint256)) internal _tierIdAfter; 66 mapping(address => mapping(uint256 => address)) internal _reservedTokenBeneficiaryOf; 75 mapping(address => mapping(uint256 => JBStored721Tier)) internal _storedTierOf; 83 mapping(address => JBTiered721Flags) internal _flagsOf; 93 mapping(address => mapping(uint256 => uint256)) internal _isTierRemoved; 104 mapping(address => uint256) internal _trackedLastSortTierIdOf; ... 119 mapping(address => uint256) public override maxTierIdOf; 129 mapping(address => mapping(address => mapping(uint256 => uint256))) public override tierBalanceOf; 138 mapping(address => mapping(uint256 => uint256)) public override numberOfReservesMintedFor; 147 mapping(address => mapping(uint256 => uint256)) public override numberOfBurnedFor; 155 mapping(address => address) public override defaultReservedTokenBeneficiaryOf; 164 mapping(address => mapping(uint256 => address)) public override firstOwnerOf; 172 mapping(address => string) public override baseUriOf; 180 mapping(address => IJBTokenUriResolver) public override tokenUriResolverOf; 188 mapping(address => string) public override contractUriOf; 197 mapping(address => mapping(uint256 => bytes32)) public override encodedIPFSUriOf;
#0 - Picodes
2022-11-08T17:31:51Z
Multiple address/ID mappings can be combined into a single mappingof an address/ID to a struct, where appropriate
Please show how
#1 - c4-judge
2022-11-08T17:31:55Z
Picodes marked the issue as grade-b