Skip to content

Latest commit

 

History

History
636 lines (364 loc) · 35 KB

SleepingBugs-Q.md

File metadata and controls

636 lines (364 loc) · 35 KB

Low

Critical address change should use two-step procedure

The critical procedures should be two step process.

See similar findings in previous Code4rena contests for reference

Recommendation

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

Functions that change important state params should emit events

block.timestamp used as time proxy

Summary

Risk of using block.timestamp for time should be considered.

Details

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.

This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.

References

SWC ID: 116

Code Snippet

Recommendation

  • Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.
  • Consider using an oracle for precision

Destination recipient for assets may be address(0)

IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

Erc20Quest.sol#L81

Erc1155Quest.sol#L54

Description

The recipient of a transfer may be address(0), leading to lost assets.

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Impact

For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments" (quote OpenZeppelin). Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts, potentially causing loss of user fund or cause the contract to malfunction completely. See.

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.

Proof of Concept

In the following context of the upgradeable contracts they are expected to use gaps for avoiding collision:

  • AccessControlUpgradeable
  • ECDSAUpgradeable
  • ERC1155BurnableUpgradeable
  • ERC1155Upgradeable
  • OwnableUpgradeable
  • IERC2981Upgradeable
  • Initializable

However, none of these contracts contain storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Refer to the bottom part of this article.

If a contract inheriting from a base contract contains additional variable, then the base contract cannot be upgraded to include any additional variable, because it would overwrite the variable declared in its child contract. This greatly limits contract upgradeability.

Code Snippet

Tools Used

Manual analysis

Recommendation

Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below.

uint256[50] private __gap;

Reference OpenZeppelin upgradeable contract templates.

Single-step process for critical ownership transfer/renounce is risky

Impact

Given that QuestFactory, RabbitHoleTickets, RabbitHoleReceipt, Quest are derived from Ownable or OwnableUpgradeable the ownership management of this contract defaults to Ownable ’s transferOwnership() and renounceOwnership() methods which are not overridden here.

Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes

Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately.

When noticed, due to a failing onlyOwner() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.

Code Snippet

Recommendation

Recommend overriding the inherited methods to null functions and use separate functions for a two-step address change:

  1. Approve a new address as a pendingOwner
  2. A transaction from the pendingOwner address claims the pending ownership change.

This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.

Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.

Missing checks for address(0x0) when assigning values to address state or immutable variables

NOTE: None of these findings where found by 4naly3er output - NC

Code Snippet

Recommendation

Check zero address before assigning or using it

Missing checks for address(0x0) on some setters

NOTE: None of these findings where found by 4naly3er output - NC

Code Snippet

Recommendation

Check zero address before assigning or using it

Informational

Solidity compiler optimizations can be problematic

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.

It's likely there is a latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Config optimizer: { enabled: true,

Constants should be defined rather than using magic numbers

NOTE: None of these findings where found by 4naly3er output - NC

Comparison of a boolean expression against a boolean value is redundant

There are instances where a boolean variable / function is checked against a boolean unnecessarily.

  • Checks can be simplified from variable == false to !variable.

  • Checks can be simplified from variable == true to variable.

  • QuestFactory.sol#L221 if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();

  • Quest.sol#L136 return claimedList[tokenId_] == true;

  • QuestFactory.sol#L73 if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Recommendation

Solve and remove TODOs

Recommendation

Use actual code

Omissions in events

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

The events should include the new value and old value where possible:

Missing 0 address check

NOTE: None of these findings where found by 4naly3er output - NC

0 address control should be done in these parts

No same value input control

Recommendation

Add a check that same value it's not used. For example:

function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
        + if (claimSignerAddress_ == claimSignerAddress) revert ADDRESS_SAME();
        claimSignerAddress = claimSignerAddress_;
}

Informationals

Unused code

Code that is not used should be removed

Recommendation

Remove the code that is not used.

Bad order of code

Clearness of the code is important for the readability and maintainability. As Solidity guidelines says about order of the layout:

  1. Type declarations
  2. State variables
  3. Events
  4. Modifiers
  5. Functions

RabbitHoleReceipt.sol#L58 modifier onlyMinter() {

Quest.sol#L76 modifier onlyAdminWithdrawAfterEnd() {

Quest.sol#L82 modifier onlyStarted() {

Quest.sol#L88 modifier onlyQuestActive() {

RabbitHoleTickets.sol#L47 modifier onlyMinter() {

Missing Natspec

NatSpec is missing for the following functions / constructor / modifiers:

QuestFactory.sol#L37 function initialize(

RabbitHoleReceipt.sol#L43 function initialize(

Quest.sol#L74

RabbitHoleTickets.sol#L32 function initialize(

ReceiptRenderer.sol#L40 function generateDataURI(

IQuest.sol function isClaimed(uint256 tokenId_) external view returns (bool);

IQuest.sol function getRewardAmount() external view returns (uint256);

IQuest.sol function getRewardToken() external view returns (address);

IQuestFactory.sol#L19 function questInfo(string memory questId_) external view returns (address, uint, uint);

Undocumented parameters

In a some places, a parameter is missing in the documentation:

QuestFactory.sol#L37 function initialize(

RabbitHoleReceipt.sol#L39 constructor() {

RabbitHoleReceipt.sol#L43 function initialize(

RabbitHoleReceipt.sol#L158 function tokenURI(

Quest.sol#L26 constructor(

Quest.sol#L74

Quest.sol#L120 /// @notice Calculate the amount of rewards

Quest.sol#L148

RabbitHoleTickets.sol#L32 function initialize(

RabbitHoleTickets.sol#L102 function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) {

Erc1155Quest.sol#L13 constructor(

ReceiptRenderer.sol#L40 function generateDataURI(

IQuest.sol function isClaimed(uint256 tokenId_) external view returns (bool);

IQuest.sol function getRewardAmount() external view returns (uint256);

IQuest.sol function getRewardToken() external view returns (address);

IQuestFactory.sol#L19 function questInfo(string memory questId_) external view returns (address, uint, uint);

Include return parameters in natspec comments

If Return parameters are declared, you must prefix them with ”/// @return”. References

Some code analysis programs do analysis by reading NatSpec details, if they can’t see the @return tag, they do incomplete analysis.

Recommendation

Include return parameters in NatSpec comments

Recommendation Code Style:

    /// @notice information about what a fooFighter function does
		/// @param fooParam what the fooParam represents
		/// @return what is the fooReturnValue returned by fooFighter
		function fooFighter(uint256 fooParam) public returns (uint256 fooReturnValue) {
      ...
    }

QuestFactory.sol#L191 /// @dev return the number of minted receipts for a quest

QuestFactory.sol#L197 /// @dev return data in the quest struct for a questId

QuestFactory.sol#L207 /// @dev recover the signer from a hash and signature

RabbitHoleReceipt.sol#L112 ) public view returns (uint[] memory) {

RabbitHoleReceipt.sol#L181 ) external view override returns (address receiver, uint256 royaltyAmount) {

RabbitHoleReceipt.sol#L190 function supportsInterface(

Quest.sol#L74

Quest.sol#L133 /// @notice Checks if a Receipt token id has been used to claim a reward

Quest.sol#L140 function getRewardAmount() public view returns (uint256) {

Quest.sol#L145 function getRewardToken() public view returns (address) {

RabbitHoleTickets.sol#L102 function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) {

RabbitHoleTickets.sol#L109 function royaltyInfo(

RabbitHoleTickets.sol#L119 function supportsInterface(

Erc20Quest.sol#L96 function protocolFee() public view returns (uint256) {

ReceiptRenderer.sol#L40 function generateDataURI(

ReceiptRenderer.sol#L82 function generateAttribute(string memory key, string memory value) public pure returns (string memory) {

ReceiptRenderer.sol#L100 function generateSVG(uint tokenId_, string memory questId_) public pure returns (string memory) {

IQuest.sol function isClaimed(uint256 tokenId_) external view returns (bool);

IQuest.sol function getRewardAmount() external view returns (uint256);

IQuest.sol function getRewardToken() external view returns (address);

IQuestFactory.sol#L19 function questInfo(string memory questId_) external view returns (address, uint, uint);

Miss leading name of modifier

Implementation doesn't only allow admins by itself to call it

/// @notice Prevents reward withdrawal until the Quest has ended
modifier onlyAdminWithdrawAfterEnd() {
if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
_;
}

Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits.