Erc1155Quest.withdrawRemainingTokens() will unexpectedly withdraw unclaimed rewards of participants

[code423n4]

Lines of code

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

Vulnerability details

Participants will not be able to claim their rewards after the quest owner calls Erc1155Quest.withdrawRemainingTokens(), which will withdraw all remaining tokens including unclaimed ones. This is different from Erc20Quest, which will retain any unclaimed rewards even after withdrawRemainingTokens() is called.

This behaviour is also different from the claim process documentation at https://user-images.githubusercontent.com/14314818/214354756-0af7e34d-746e-4429-8b55-8eb6d8bb1e31.png

Proof of Concept

Suppose the ERC1155 quest has ended and the quest owner calls withdrawRemainingTokens() to transfer out all the remaining tokens in the contract. Now, participants who still has unredeemed RH receipts will not be able to claim their rewards from the contract as the balance is zero.

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

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

Recommended Mitigation Steps

Rectify the withdrawRemainingTokens() to only withdraw non-claimable rewards (based on proportion of unminted RH receipts), while leaving the unclaimed rewards in the 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)