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

[code423n4]

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.

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate of #22

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)

[c4-judge]

kirk-baird marked the issue as satisfactory