Platform: Code4rena
Start Date: 25/01/2023
Pot Size: $36,500 USDC
Total HM: 11
Participants: 173
Period: 5 days
Judge: kirk-baird
Total Solo HM: 1
Id: 208
League: ETH
Rank: 35/173
Findings: 2
Award: $130.93
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: CodingNameKiki
Also found by: 0x1f8b, 0x4non, 0x5rings, 0xAgro, 0xMirce, 0xRobocop, 0xSmartContract, 0xackermann, AkshaySrivastav, ArmedGoose, Aymen0909, BClabs, Breeje, Dewaxindo, Diana, HollaDieWaldfee, IceBear, IllIllI, Iurii3, Josiah, Krayt, PaludoX0, Phenomana, PrasadLak, RaymondFam, Rolezn, SaeedAlipoor01988, SaharDevep, SleepingBugs, adriro, arialblack14, bin2chen, brevis, btk, carlitox477, carrotsmuggler, catellatech, chaduke, chrisdior4, cryptonue, cryptostellar5, csanuragjain, ddimitrov22, fellows, glcanvas, halden, hl_, horsefacts, jat, joestakey, kenta, libratus, lukris02, luxartvinsec, manikantanynala97, martin, matrix_0wl, nadin, nicobevi, oberon, peanuts, prestoncodes, rbserver, sakshamguruji, sashik_eth, sayan, seeu, thekmj, tnevler, trustindistrust, tsvetanovv, xAriextz, zadaru13, zaskoh
119.6013 USDC - $119.60
Our Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it. This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.
import {contract1 , contract2} from "filename.sol";
POC:
4: import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; 10: import '@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol'; 11: import '@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol';
4-13: import '@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol'; import '@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol'; import '@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol'; import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; import '@openzeppelin/contracts-upgradeable/interfaces/IERC2981Upgradeable.sol'; import '@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol'; import './ReceiptRenderer.sol'; import './interfaces/IQuestFactory.sol'; import './interfaces/IQuest.sol';
4-9: import '@openzeppelin/contracts-upgradeable/token/ERC1155/ERC1155Upgradeable.sol'; import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; import '@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155BurnableUpgradeable.sol'; import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; import '@openzeppelin/contracts-upgradeable/interfaces/IERC2981Upgradeable.sol'; import './TicketRenderer.sol';
4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
53: return (maxTotalRewards() * questFee) / 10_000; 97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
48: setQuestFee(2_000); 187: if (questFee_ > 10_000) revert QuestFeeTooHigh();
184: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
113: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
17: bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
53: return (maxTotalRewards() * questFee) / 10_000; 97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
187: if (questFee_ > 10_000) revert QuestFeeTooHigh();
184: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
113: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
Number of Instances Identified: 17
96: function protocolFee() public view returns (uint256)
122: function _calculateRewards(uint256 redeemableTokenCount_) internal virtual returns (uint256) 135: function isClaimed(uint256 tokenId_) public view returns (bool) 140: function getRewardAmount() public view returns (uint256) 145: function getRewardToken() public view returns (address)
193: function getNumberMinted(string memory questId_) external view returns (uint) 199: function questInfo(string memory questId_) external view returns (address, uint, uint) 210: function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address)
109: function getOwnedTokenIdsOfQuest() 158: function tokenURI() 181: function royaltyInfo() 192: function supportsInterface()
102: function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) 112: function royaltyInfo() 119: function supportsInterface()
40: function generateDataURI() 82: function generateAttribute(string memory key, string memory value) public pure returns (string memory)
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
If Return parameters are declared, you must prefix them with /// @return.
Some code analysis programs do analysis by reading NatSpec details, if they can’t see the @return tag, they do incomplete analysis.
Include return parameters in NatSpec comments
Recommendation Natspec Code Style:
/// @return Returns a page's URI if it has been minted
Number of Instances Identified: 16
The functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
81: function withdrawRemainingTokens(address to_) public override onlyOwner
50: function start() public virtual onlyOwner 57: function pause() public onlyOwner onlyStarted 63: function unPause() public onlyOwner onlyStarted 150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd
142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner 159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner 165: function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner 172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner 179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner
65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner 71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner 77: function setQuestFactory(address questFactory_) public onlyOwner
54: function setTicketRenderer(address ticketRenderer_) public onlyOwner 60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner 83: function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter
__GAP[50] STORAGE VARIABLE TO ALLOW FOR NEW STORAGE VARIABLES IN LATER VERSIONSNumber of Instances Identified: 3
See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
POC:
16: contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory
15-21: contract RabbitHoleReceipt is Initializable, ERC721Upgradeable, ERC721EnumerableUpgradeable, ERC721URIStorageUpgradeable, OwnableUpgradeable, IERC2981Upgradeable
11-16: contract RabbitHoleTickets is Initializable, ERC1155Upgradeable, OwnableUpgradeable, ERC1155BurnableUpgradeable, IERC2981Upgradeable
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
4: // TODO clean this whole thing up
When deploying contracts, you should use the latest released version of Solidity. Apart from exceptional cases, only the latest version receives security fixes. Furthermore, breaking changes as well as new features are introduced regularly. Latest Version is 0.8.17
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
n 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. https://swcregistry.io/docs/SWC-103
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
178-181: function royaltyInfo( uint256 tokenId_, uint256 salePrice_ ) external view override returns (address receiver, uint256 royaltyAmount)
109-112: function royaltyInfo( uint256 tokenId_, uint256 salePrice_ ) external view override returns (address receiver, uint256 royaltyAmount)
#0 - c4-sponsor
2023-02-07T23:19:47Z
waynehoover marked the issue as sponsor acknowledged
#1 - kirk-baird
2023-02-09T05:21:07Z
Downgraded #651 and #630
#2 - c4-judge
2023-02-14T09:51:34Z
kirk-baird marked the issue as grade-a
#3 - c4-judge
2023-02-21T07:15:33Z
kirk-baird marked the issue as grade-b
#4 - kirk-baird
2023-02-21T07:16:04Z
Comments to improve this report
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xAgro, 0xSmartContract, 0xhacksmithh, 0xngndev, Aymen0909, Bnke0x0, Breeje, Deivitto, Diana, Dug, Iurii3, LethL, MiniGlome, NoamYakov, RaymondFam, ReyAdmirado, Rolezn, SAAJ, adriro, ali, arialblack14, atharvasama, c3phas, carlitox477, catellatech, chaduke, cryptonue, cryptostellar5, ddimitrov22, dharma09, doublesharp, favelanky, georgits, glcanvas, gzeon, halden, horsefacts, jasonxiale, joestakey, karanctf, lukris02, matrix_0wl, nadin, navinavu, saneryee, shark, thekmj
11.3269 USDC - $11.33
Number of Instances Identified: 4
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.
70: for (uint i = 0; i < tokenIds_.length; i++) 104: for (uint i = 0; i < tokens.length; i++)
117: for (uint i = 0; i < msgSenderBalance; i++) 128: for (uint i = 0; i < msgSenderBalance; i++)
Number of Instances Identified: 2
Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.
Each MLOAD and MSTORE costs 3 additional gas
178-181: function royaltyInfo( uint256 tokenId_, uint256 salePrice_ ) external view override returns (address receiver, uint256 royaltyAmount)
109-112: function royaltyInfo( uint256 tokenId_, uint256 salePrice_ ) external view override returns (address receiver, uint256 royaltyAmount)
Number of Instances Identified: 1
Using the addition operator instead of plus-equals saves 113 gas
115: redeemedTokens += redeemableTokenCount;
Number of Instances Identified: 2
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
136: return claimedList[tokenId_] == true;
221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
Number of Instances Identified: 3
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
66: function _transferRewards(uint256 amount_) internal 74: function _calculateRewards(uint256 redeemableTokenCount_) internal
152: function grantDefaultAdminAndCreateQuestRole(address account_) internal
#0 - c4-judge
2023-02-16T06:32:40Z
kirk-baird marked the issue as grade-b