RabbitHole Quest Protocol contest - catellatech's results

C4 Quest Protocol QA Report

Unspecific Compiler Version Pragma

Description

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Impact

Issue Information: [L001]

Findings:

quest-protocol\contracts\Erc1155Quest.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\Erc20Quest.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\Quest.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\QuestFactory.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\RabbitHoleReceipt.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\RabbitHoleTickets.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\ReceiptRenderer.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\TicketRenderer.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\interfaces\IQuest.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\interfaces\IQuestFactory.sol::2 => pragma solidity ^0.8.15;
quest-protocol\contracts\test\SampleErc1155.sol::2 => pragma solidity ^0.8.15;

Non-Critial Issues

Impact

Issue Information: [NC-001] - Functions Mutating Storage Should Emit Events

Description

Functions mutating storage should emit an Event for easy off-chain monitoring.

Findings:

quest-protocol\contracts\Erc1155Quest.sol::41 =>  function _transferRewards(uint256 amount_);
quest-protocol\contracts\Erc1155Quest.sol::102 => function withdrawFee() public onlyAdminWithdrawAfterEnd;
quest-protocol\contracts\Erc1155Quest.sol::33 => function start() public override;

QA

Impact

Issue Information: [QA-001] - transferOwnership should be two step process

Description

"QuestFactory.sol" inherit OpenZeppelin's OwnableUpgradeable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling newQuest.transferOwnership(msg.sender) If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Example : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

The contract have many onlyOwner function and the  contract is inherited from the Ownable which includes transferOwnership. Recommended Mitigation Steps
Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

C4 Quest Protocol Gas Report

Files analyzed

• quest-protocol\contracts\Erc1155Quest.sol
• quest-protocol\contracts\Erc20Quest.sol
• quest-protocol\contracts\Quest.sol
• quest-protocol\contracts\QuestFactory.sol
• quest-protocol\contracts\RabbitHoleReceipt.sol
• quest-protocol\contracts\RabbitHoleTickets.sol
• quest-protocol\contracts\ReceiptRenderer.sol
• quest-protocol\contracts\TicketRenderer.sol

Issues found

Don't Initialize Variables with Default Value

Impact

Issue Information: [G001]

Findings:

quest-protocol\contracts\Quest.sol::103 => uint256 redeemableTokenCount = 0;
quest-protocol\contracts\RabbitHoleReceipt.sol::115 => uint foundTokens = 0;
quest-protocol\contracts\RabbitHoleReceipt.sol::126 => uint filterTokensIndexTracker = 0;

Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

Issue Information: [G002]

Findings:

quest-protocol\contracts\RabbitHoleReceipt.sol::129 => if (tokenIdsForQuest[i] > 0) {

Use immutable for OpenZeppelin AccessControl's Roles Declarations

Impact

Issue Information: [G003]

Findings:

quest-protocol\contracts\QuestFactory.sol::17 => bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
quest-protocol\contracts\QuestFactory.sol::211 => bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));

Use calldata Instead of Memory for Function Parameters

Impact

Issue Information: [G004]

Findings:

quest-protocol\contracts\Quest.sol::2 =>     function _setClaimed(uint256[] memory tokenIds_);

INCREMENTS CAN BE UNCHECKED

Description

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Impact

Issue Information: [G005]

Findings:

quest-protocol\contracts\Quest.sol::70 =>   for (uint i = 0; i < tokenIds_.length; i++);
quest-protocol\contracts\Quest.sol::104 =>  for (uint i = 0; i < tokens.length; i++);
quest-protocol\contracts\RabbitHoleReceipt.sol::117 => for (uint i = 0; i < msgSenderBalance; i++);
quest-protocol\contracts\RabbitHoleReceipt.sol::128 => for (uint i = 0; i < msgSenderBalance; i++);

The code would go from:

for (uint i = 0; i < tokenIds_.length; i++) {.....}

to:

for (uint i = 0; i < tokenIds_.length;) {  
 // ...  
 unchecked { ++i; }  
}