In ERC1155 quests the owner withdraws all of the remaining tokens even for the unclaimed receipts. Leaving users who didn't claim their receipts before the quest end time unable to claim rewards.

[code423n4]

Lines of code

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

Vulnerability details

Impact

In ERC1155 quests the owner is able to withdraw all of the remaining tokens even for the unclaimed receipts.
This problem prevents users from claiming their rewards, as a receipt can be claimed only on its allocated quest.

Proof of Concept

In ERC1155 quests, the function withdrawRemainingTokens is called by the owner to withdraw the remaining tokens.
The problem here is that the owner withdraws all of the ERC1155 tokens even the unclaimed ones, which are allocated to the users that finished the quest. As there is no check to see how many receipts were minted for this particular quest and how many receipts were claimed. The owner is able to withdraw all of the remaining tokens, preventing users from claiming their rewards as a minted receipt can be claimed only on its allocated quest contract.

Example:
We have 4 people - Jake, Finn, Alice and Kiki

1. All of the people above finished the quest and received minted receipts.
2. But only Jake and Finn claimed their receipts before the quest end time.
3. The owner calls the function withdrawRemainingTokens and withdraws all of the tokens, as there is no check to see how many receipts were minted and how many were actually claimed.
4. Leaving both Alice and Kiki unable to claim rewards, as their receipts can be used only on this quest contract.

This problem prevents users from claiming their rewards, as the owner isn't supposed to withdraw the tokens allocated for the unclaimed receipts after the quest end time.

contracts/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:    }

You can see that this is enforced in the ERC20 quests and the tokens for the unclaimed receipts can't be withdrawn by the owner at the end of the quest. This is clearly stated as well in the function comment on L79 - minted receipts should still be able to claim rewards even after the quest end time and can't be withdrawn by the owner.

contracts/Erc20Quest.sol

79:  /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn).

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:    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the function receiptRedeemers to Erc1155Quest.sol, which checks how many people actually finished the quest and got minted receipts. And refactor the function withdrawRemainingTokens to withdraw the ERC1155 tokens without the unclaimed ones:

contracts/Erc1155Quest.sol

+  function receiptRedeemers() public view returns (uint256) {
+         return questFactoryContract.getNumberMinted(questId);
+     }

54:  function withdrawRemainingTokens(address to_) public override onlyOwner {
55:        super.withdrawRemainingTokens(to_);
+          uint256 unclaimedTokens = receiptRedeemers() - redeemedTokens;

56:        IERC1155(rewardToken).safeTransferFrom(
57:            address(this),
58:            to_,
59:            rewardAmountInWeiOrTokenId,
60:            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedTokens,
61:            '0x00'
62:        );
63:    }

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

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