QuestFactory.sol: receipts can be minted after endTime of a quest is reached which can lead to loss of rewards for users

[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/QuestFactory.sol#L219-L229

Vulnerability details

Impact

When the endTime is reached, any unclaimed tokens can be withdrawn from the Erc20Quest contract via the withdrawRemainingTokens function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87).

The endTime is checked in the onlyAdminWithdrawAfterEnd modifier in the parent function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L150).

This is the calculation to determine the amount to withdraw:

uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

You can see that the rewards for any minted receipts that are not yet redeemed will stay in the contract.

The issue with this is that there is no check in the QuestFactory.mintReceipt function that makes sure that receipts cannot be minted after the endTime of a quest is reached (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L229).

Therefore when Erc20Quest.withdrawRemainingTokens is called and then a new receipt is minted, there won't be enough rewards in the Erc20Quest contract for everyone to redeem.

The sponsor provided the information that the claimSigner will sign mintReceipt requests only when the endTime is not yet reached.

I think however that this is not sufficient and the endTime check must occur on-chain in the QuestFactory.mintReceipt function.

This is because a mintReceipt request might be signed off-chain in time (i.e. before the endTime) but is only processed on-chain after the endTime.

Proof of Concept

Think of the following scenario:

1. The claimSigner signs a mintReceipt request for User A before the endTime is reached
2. Now the endTime is reached and all remaining funds are withdrawn from the Erc20Quest contract. Say there is funds for one receipt left in the contract which can be redeemed by User B
3. Now the signed mintReceipt request is processed on-chain and User A redeems his reward
4. User B can now no longer redeem his reward because there are no funds in the Erc20Quest contract

You can see from this scenario how - when a mintReceipt request is processed on-chain after the endTime - users can miss out on their rewards.

Tools Used

VSCode

Recommended Mitigation Steps

The QuestFactory contract should have access to the endTime of each Quest and not mint receipts after the endTime has been reached.

This can be achieved by extending the Quest struct to include the endTime:

struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint totalParticipants;
        uint numberMinted;
        uint endTime;
    }

The endTime can then be checked in the QuestFactory.mintReceipt function:

if (quests[questId_].endTime <= block.timestamp) revert QuestExpired();

[c4-judge]

kirk-baird marked the issue as duplicate of #22

[c4-judge]

kirk-baird marked the issue as satisfactory