Quest owner of Erc1155Quest contract can withdraw all reward tokens after the Quest ends, resulting in loss to unclaimed users

[code423n4]

Lines of code

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

Vulnerability details

Impact

withdrawRemainingTokens() function allows the withdrawal of the whole Erc1155Quest contract balance. This means any users who haven't yet claimed their rewards will be unable to do so resulting in a fund loss.

Proof of Concept

withdrawRemainingTokens function looks like this:

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

You can see that the amount withdrawn is given by IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) which is the contract balance for that particular tokenId.

If some users haven't claimed their rewards yet, they will be unable to do so after the withdrawRemainingTokens is called.

In the Erc20Quest contract the withdrawRemainingTokens function calculates the nonClaimableTokens which is the allowed amount of tokens to withdraw. A similar logic should be applied, but is missing in the 1155 contract.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

I suggest, the nonClaimableTokens to be calculated and adjusted when calculating how much tokens to withdraw after Quest ends. The logic from Erc20Quest contract can be used here as well.

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