Unclaimed rewards in Erc1155Quest will be lost for users

[code423n4]

Lines of code

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

Vulnerability details

Unclaimed rewards in Erc1155Quest will be lost for users after withdrawRemainingTokens() was called

The Erc1155Quest.withdrawRemainingTokens(address to_) is for an admin to withdraw any additional tokens that are too much in the contract.

The current implementation just transfers all the holdings of the contract. It is not reducing the amount of all the users that are allowed to claim rewards and so the users can't claim their rewards after the Admin called the withdrawRemainingTokens function.

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

Even if we trust and assume the current Rabbit team will return or move funds to the users that didn't claimed, with a growing allowed list of addresses that have the CREATE_QUEST_ROLE role to start new quests it get's more and more possible that there will be a malicious actor that will not transfer the funds to the users.

Proof of Concept

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

The test shows, that an users is not able to claim their rewards, after the admin called withdrawRemainingTokens.

Add: File: test/Erc1155Quest.spec.ts

describe('claim() for user should still be possible after withdrawRemainingTokens() was falled from admin', async () => {    
    it('it should be possible to claim the correct amount after the admin has called withdrawRemainingTokens()', async () => {
        await deployedRabbitholeReceiptContract.mint(owner.address, questId)
        await deployedQuestContract.start()

        await ethers.provider.send('evm_increaseTime', [86400])

        expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

        const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, owner.address)
        expect(totalTokens.length).to.equal(1)

        expect(await deployedQuestContract.isClaimed(1)).to.equal(false)
        
        // owner calles withdrawRemainingTokens before user calls claim
        await deployedQuestContract.connect(owner).withdrawRemainingTokens(firstAddress.address)

        await deployedQuestContract.claim() // @audit this now failes with ERC1155: insufficient balance for transfer but should transfer it to the user
        expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(1)
        await ethers.provider.send('evm_increaseTime', [-86400])
    })
})

Tools Used

manual review

Recommended Mitigation Steps

Reduce from the total allowed claimers the amount that are already claimed, and reduce this amount from the current balance.
Like in the Erc20Quest.sol implementation.

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