Erc1155Quest.sol: withdrawRemainingTokens function withdraws unclaimed rewards

[code423n4]

Lines of code

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

Vulnerability details

Impact

The Erc1155Quest.withdrawRemainingTokens function is used to withdraw the remaining funds from the contract after the quest has ended (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63).

It is intended that users that have a valid receipt can claim their reward even after the quest has ended. This was confirmed by the sponsor.

The issue is that the Erc1155Quest.withdrawRemainingTokens function withdraws ALL remaining tokens and does not leave unclaimed tokens in the contract. This means that any users that want to claim their token after Erc1155Quest.withdrawRemainingTokens was called cannot do so anymore because there are no tokens left.

You can see in the Erc20Quest.withdrawRemainingTokens function (which is implemented correctly) how it should work:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L84-L86

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

The unclaimed tokens should remain in the contract.

Proof of Concept

1. A user has a valid receipt to withdraw a reward token from the Erc1155Quest contract
2. The endTime is reached and the owner calls Erc1155Quest.withdrawRemainingTokens which transfers all reward tokens to the to address
3. The user should still be able to claim his reward token but there are no reward tokens left in the contract

Tools Used

VSCode

Recommended Mitigation Steps

The Erc1155Quest.withdrawRemainingTokens function should make sure that receiptRedeemers() - redeemedTokens tokens remain in the contract.

Therefore it also needs to implement the receiptReedemers() function which can be the same as in the Erc20Quest contract.

[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)