Erc1155Quest.withdrawRemainingTokens() will withdraw all remaining tokens including unclaimed tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

Quest.claim() will always revert after Erc1155Quest.withdrawRemainingTokens() has been called.
Unclaimed users/receipts will not be able to claim, their rewards will lose.

Proof of Concept

Erc1155Quest.withdrawRemainingTokens() will withdraw all remaining tokens including unclaimed tokens:

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

Therefore, after it's called, anyone calling claim() will revert because there are no reward tokens left in the contract.

While the Erc20Quest.withdrawRemainingTokens() will leave the unclaimedTokens in the contract for claim():

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);

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

Tools Used

VS Code

Recommended Mitigation Steps

We should leave the unclaimed tokens in the contract when calling Erc1155Quest.withdrawRemainingTokens(), just as the Erc20Quest.withdrawRemainingTokens() did.

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