code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

After completing their tasks users can mint a new receipt token which they can later claim reward with it using the claim function, this function can not be called when the Quest contract is paused so the users can't claim when quest contract is paused.

After the end of the quest the owner is able to withdraw back the remaining tokens, in the case of Erc20Quest the owner is able to withdraw only the reward tokens for the non-participants and the participants rewards are kept in the contract until they get claimed but for the Erc1155Quest the owner is able to withdraw all tokens deposited when starting the quest regardless of the already minted receipt.

Thus for an Erc1155Quest type quest the scenario where users can not claim their reward is the following :

the owner start the Erc1155Quest by calling the start() function and then he set the contract to paused.
The users can complete the tasks and mint a receipt token by calling the mintReceipt function.
users can not claim reward for their receipt token because the Quest contract is paused.
the owner wait until the end of the quest and then calls the withdrawRemainingTokens function to get back all the tokens previously deposited at the start.
in the end the users did complete their tasks but they will never receive the reward for it.

Proof of Concept

This issue occurs because withdrawRemainingTokens function of the Erc1155Quest contract allow the owner to withdraw all the tokens deposited at the start :

File: Erc1155Quest.sol Line 54-63

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
        '0x00'
    );
}

On the contrary to the withdrawRemainingTokens function of the Erc20Quest which account for the already minted tokens by calling questFactoryContract.getNumberMinted(questId) and thus allow the owner to only withdraw reward correspanding to non-minted tokens, the withdrawRemainingTokens function of the Erc1155Quest allow the owner to withdraw all the reward tokens regardless if they correspand to a minted receipt.

This will allow the owner to follow the scenario mentioned before to stop users for claiming their rewards.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue i recommend to modify the withdrawRemainingTokens function of the Erc1155Quest and make work similarly to the one in Erc20Quest.