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: 39/173
Findings: 2
Award: $122.19
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: adriro
Also found by: 0xMAKEOUTHILL, 0xMirce, 7siech, AkshaySrivastav, AlexCzm, Awesome, Aymen0909, Cryptor, Deivitto, DimitarDimitrov, ElKu, Garrett, Jayus, Josiah, Kenshin, KrisApostolov, RaymondFam, SovaSlava, Timenov, UdarTeam, amaechieth, btk, c3phas, codeislight, fellows, frankudoags, gzeon, hansfriese, luxartvinsec, millersplanet, mookimgo, navinavu, oberon, paspe, pavankv, petersspetrov, pfapostol, prestoncodes, rbserver, sakshamguruji, shark, thekmj, trustindistrust, tsvetanovv, usmannk, vagrant, vanko1, xAriextz, yosuke
2.5852 USDC - $2.59
Judge has assessed an item in Issue #71 as 3 risk. The relevant finding follows:
[L-04] onlyMinter() modifier is not working as expected Description onlyMinter() can be bypasssed by anyone due to an invalid check:
modifier onlyMinter() { msg.sender == minterAddress; _; }
Thus, everyone can mint tokens:
function mint(address to_, string memory questId_) public onlyMinter { _tokenIds.increment(); uint newTokenID = _tokenIds.current(); questIdForTokenId[newTokenID] = questId_; timestampForTokenId[newTokenID] = block.timestamp; _safeMint(to_, newTokenID); }
Lines of code RabbitHoleReceipt.sol#L58 RabbitHoleTickets.sol#L47 Recommended Mitigation Steps Replace the affected modifier by this one:
modifier onlyMinter() { require(msg.sender == minterAddress, "Only minter can mint tokens"); _; }
#0 - c4-judge
2023-02-21T08:00:11Z
kirk-baird marked the issue as duplicate of #608
#1 - c4-judge
2023-02-21T08:04:08Z
kirk-baird marked the issue as satisfactory
🌟 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
| Total Low issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [L-01] | Address(0) checks | 6 |
| [L-02] | Missing Event for critical changes | 15 |
| [L-03] | Loss of precision due to rounding | 4 |
| [L-04] | onlyMinter() modifier is not working as expected | 2 |
| [L-05] | Critical changes should use two-step procedure | 2 |
| [L-06] | No storage gap for upgradeable contracts | 3 |
| [L-07] | royaltyFee is not capped at 100% | 2 |
| [L-08] | Protocol is using deprecated npm dependency (axios) | 1 |
| [L-09] | Value is not validated to be different than the existing one | 4 |
| [L-10] | Add a timelock to critical functions | 3 |
| Total Non-Critical issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [NC-01] | Use uint256 instead uint | 33 |
| [NC-02] | Lock pragmas to specific compiler version | All Contracts |
| [NC-03] | Use a more recent version of solidity | All Contracts |
| [NC-04] | Constants in comparisons should appear on the left side | 2 |
| [NC-05] | Solidity compiler optimizations can be problematic | 1 |
| [NC-06] | Use immutable instead of constant for values such as a call to keccak256() | 1 |
| [NC-07] | Events that mark critical parameter changes should contain both the old and the new value | 4 |
| [NC-08] | Use bytes.concat() and string.concat() | 16 |
| [NC-09] | Non-usage of specific imports | All Contracts |
| [NC-10] | Mark visibility of initialize() functions as external | 3 |
| [NC-11] | Include @return parameters in NatSpec comments | All Contracts |
| [NC-12] | Function writing does not comply with the Solidity Style Guide | All Contracts |
| [NC-13] | Lines are too long | 6 |
| [NC-14] | NatSpec comments should be increased in contracts | All Contracts |
| [NC-15] | For functions, follow Solidity standard naming conventions | All Contracts |
| [NC-16] | Contracts should have full test coverage | All Contracts |
| [NC-17] | Critical changes should use-two step procedure | 3 |
| [NC-18] | Generate perfect code headers every time | All Contracts |
| [NC-19] | There is no need to cast a variable that is already an address, such as address(x) | 2 |
| [NC-20] | Add NatSpec comment to mapping | 6 |
Address(0) checksCheck of address(0) to protect the code from (0x0000000000000000000000000000000000000000) address problem just in case. This is best practice or instead of suggesting that they verify _address != address(0), you could add some good NatSpec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.
function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { claimSignerAddress = claimSignerAddress_; }
function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { if (claimSignerAddress_ == address(0)) revert(); claimSignerAddress = claimSignerAddress_; }
Events help non-contract tools to track changes, and events prevent users from being surprised by changes.
function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner { if (protocolFeeRecipient_ == address(0)) revert AddressZeroNotAllowed(); protocolFeeRecipient = protocolFeeRecipient_; }
Emit event for critical changes.
Loss of precision due to the nature of arithmetics and rounding errors.
uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
Add scalars so roundings are negligible.
onlyMinter() modifier is not working as expectedonlyMinter() can be bypasssed by anyone due to an invalid check:
modifier onlyMinter() { msg.sender == minterAddress; _; }
Thus, everyone can mint tokens:
function mint(address to_, string memory questId_) public onlyMinter { _tokenIds.increment(); uint newTokenID = _tokenIds.current(); questIdForTokenId[newTokenID] = questId_; timestampForTokenId[newTokenID] = block.timestamp; _safeMint(to_, newTokenID); }
Replace the affected modifier by this one:
modifier onlyMinter() { require(msg.sender == minterAddress, "Only minter can mint tokens"); _; }
The protocol inherit openzeppelin OwnableUpgradeable.sol which does not use two-step procedure when transferring ownership, and since the protocol rely heavily on the onlyOwner() modifier (24 results on 8 files), Thus using two-step procedure is a best practice
in case of transferring the ownership to an invalid address.
function transferOwnership(address newOwner) public virtual onlyOwner { require(newOwner != address(0), "Ownable: new owner is the zero address"); _transferOwnership(newOwner); } function _transferOwnership(address newOwner) internal virtual { address oldOwner = _owner; _owner = newOwner; emit OwnershipTransferred(oldOwner, newOwner); }
Consider adding two step procedure on the critical functions where the first is announcing a pending owner and the new address should then claim its ownership, or use Ownable2StepUpgradeable.sol instead.
For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.
See this for a description of this storage variable.
Consider adding a storage gap at the end of the upgradeable contract:
/** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ uint256[50] private __gap;
royaltyFee is not capped at 100%The royaltyFee is not capped at 100%, Thus it may exceed the salePrice which is not fully compatible with the ERC-2981 resulting in:
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { royaltyFee = royaltyFee_; emit RoyaltyFeeSet(royaltyFee_); }
Add a feeDenominator() function:
/** * @dev The denominator with which to interpret the fee set in {_setTokenRoyalty} and {_setDefaultRoyalty} as a * fraction of the sale price. Defaults to 10000 so fees are expressed in basis points, but may be customized by an * override. */ function _feeDenominator() internal pure virtual returns (uint96) { return 10000; }
and cap the royaltyFee to 100% :
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { require(royaltyFee_ <= _feeDenominator(), "ERC2981: royalty fee will exceed salePrice") royaltyFee = royaltyFee_; emit RoyaltyFeeSet(royaltyFee_); }
npm dependency (axios)The protocol is using deprecated npm dependency (axios : 0.20.0)
"axios": "^0.20.0",
Upgrade axios to version 0.21.3 or higher.
While the value is validated to be in the constant bounds, it is not validated to be different than the existing one. Queueing the same value will cause multiple abnormal events to be emitted, will ultimately result in a no-op situation.
function setMinterAddress(address minterAddress_) public onlyOwner { minterAddress = minterAddress_; emit MinterAddressSet(minterAddress_); }
function setMinterAddress(address minterAddress_) public onlyOwner { require(minterAddress_ != minterAddress, "Address the same as before") minterAddress = minterAddress_; emit MinterAddressSet(minterAddress_); }
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { royaltyFee = royaltyFee_; emit RoyaltyFeeSet(royaltyFee_); }
Consider adding a timelock to the critical changes.
uint256 instead uint33 results in 7 files.
Some developers prefer to use uint256 because it is consistent with other uint data types, which also specify their size, and also because making the size of the data explicit reminds the developer and the reader how much data they've got to play with, which may help prevent or detect bugs.
For example if doing bytes4(keccak('transfer(address, uint)’)), you'll get a different method sig ID than bytes4(keccak('transfer(address, uint256)’)) and smart contracts will only understand the latter when comparing method sig IDs.
Use uint256 instead uint.
Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.
pragma solidity ^0.8.15;
Ethereum Smart Contract Best Practices: Lock pragmas to specific compiler version.
For security, it is best practice to use the latest Solidity version.
pragma solidity ^0.8.15;
Old version of Solidity is used (0.8.15), newer version can be used (0.8.17).
Constants in comparisons should appear on the left side, doing so will prevent typo bug.
if (tokens.length == 0) revert NoTokensToClaim();
if (redeemableTokenCount == 0) revert AlreadyClaimed();
if (0 == tokens.length) revert NoTokensToClaim();
if (0 == redeemableTokenCount) revert AlreadyClaimed();
Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.
Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.
Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.
Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.
solidity: { compilers: [ { version: '0.8.15', settings: { optimizer: { enabled: true, runs: 5000, }, },
Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.
immutable instead of constant for values such as a call to keccak256()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.
bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
Use immutable instead of constant.
Events that mark critical parameter changes should contain both the old and the new value.
function setMinterAddress(address minterAddress_) public onlyOwner { minterAddress = minterAddress_; emit MinterAddressSet(minterAddress_); }
event MinterAddressSet(address indexed previousMinterAddress, address indexed minterAddress); function setMinterAddress(address minterAddress_) public onlyOwner { emit MinterAddressSet(minterAddress, minterAddress_); minterAddress = minterAddress_; }
bytes.concat() and string.concat()bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)string.concat() vs abi.encodePacked(<string>,<string>)Use bytes.concat() and string.concat() instead.
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation.
The Solidity docs recommend specifying imported symbols explicitly.
Ref: https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
Use specific imports syntax per solidity docs recommendation.
initialize() functions as externalIf someone wants to extend via inheritance, it might make more sense that the overridden initialize() function calls the internal {...}_init function, not the parent public initialize() function.
External instead of public would give more the sense of the initialize() functions to behave like a constructor (only called on deployment, so should only be called externally)
Security point of view, it might be safer so that it cannot be called internally by accident in the child contract
It might cost a bit less gas to use external over public
It is possible to override a function from external to public ("opening it up") but it is not possible to override a function from public to external ("narrow it down").
Ref: OpenZeppelin/openzeppelin-contracts#3750
function initialize( address ticketRenderer_, address royaltyRecipient_, address minterAddress_, uint royaltyFee_ ) public initializer { __ERC1155_init(''); __Ownable_init(); __ERC1155Burnable_init(); royaltyRecipient = royaltyRecipient_; minterAddress = minterAddress_; royaltyFee = royaltyFee_; TicketRendererContract = TicketRenderer(ticketRenderer_); }
Change the visibility of initialize() functions to external
@return parameters in NatSpec commentsIf 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 the @return argument in the NatSpec comments.
Solidity Style GuideOrdering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
Functions should be grouped according to their visibility and ordered:
constructor()receive()fallback()external / public / internal / privateview / pureFollow Solidity Style Guide.
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.
Ref: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Split the long lines when they reach the max length.
It is recommended that Solidity contracts are fully annotated using NatSpec, 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.
Some code analysis programs do analysis by reading NatSpec details, if they can't see the tags (@param, @dev, @return), they do incomplete analysis.
Include NatSpec comments in the codebase.
The protocol don't follow solidity standard naming convention.
Ref: https://docs.soliditylang.org/en/v0.8.17/style-guide.html#naming-conventions
Follow solidity standard naming convention.
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
- What is the overall line coverage percentage provided by your tests?: 89
Line coverage percentage should be 100%.
Critical changes should use-two step procedure.
function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner { if (protocolFeeRecipient_ == address(0)) revert AddressZeroNotAllowed(); protocolFeeRecipient = protocolFeeRecipient_; }
Consider adding two step procedure on the critical functions where the first is announcing a pending address and the new address should then claim its role.
I recommend using header for Solidity code layout and readability.
/*////////////////////////////////////////////////////////////// TESTING 123 //////////////////////////////////////////////////////////////*/
address(x)There is no need to cast a variable that is already an address, such as address(x), x is also address.
Erc1155Quest newQuest = new Erc1155Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountOrTokenId_, questId_, address(rabbitholeReceiptContract) );
Use directly variable :
Erc1155Quest newQuest = new Erc1155Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountOrTokenId_, questId_, rabbitholeReceiptContract );
mappingAdd NatSpec comments describing mapping keys and values.
mapping(uint => uint) public timestampForTokenId;
/// @dev uint(timestamp) => uint(token Id) mapping(uint => uint) public timestampForTokenId;
#0 - c4-sponsor
2023-02-08T14:52:20Z
GarrettJMU marked the issue as sponsor confirmed
#1 - c4-judge
2023-02-16T07:32:21Z
kirk-baird marked the issue as grade-a