ERC1155Quest contract owner could withdraw all users rewards when the quest has ended, if users don't claim a reward before the end

[code423n4]

Lines of code

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

Vulnerability details

Impact

When the quest ended, the ERC1155Quest contract owner could withdraw all remaining tokens, including users' reward tokens which are not claimed before the end. Because of that, if the user wants to claim their rewards after the quest end and after the ERC1155Quest contract owner calls a function withdrawRemainingTokens it will get an error that there are not enough tokens at the ERC1155Quest contract and not possible to claim a reward.

On the other side, in the ERC20Quest contract by lines Erc20Quest.sol#L84:L86 there is calculation how many tokens are not claimed, so the contract owner will get the difference between current contract balance and not claimed rewards. Because of that calculation, users could claim their reward after the ERC20Quest ended and the contract owner calls a function withdrawRemainingTokens.

Proof of Concept

Add two tests in Erc1155Quest.spec.ts

it('after quest end, should claim and withdraw remaining tokens', async () => {
    await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
    await deployedQuestContract.start()

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

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

    await deployedQuestContract.connect(firstAddress).claim()

    await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

    expect(await deployedSampleErc1155Contract.balanceOf(firstAddress.address, rewardAmount)).to.equal(1)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(99)
    await ethers.provider.send('evm_increaseTime', [-86400])
})

it('after quest end, should withdraw remaining tokens and cannot claim', async () => {
    await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
    await deployedQuestContract.start()

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

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

    await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(100)
    await expect(
    deployedQuestContract.connect(firstAddress).claim()
    ).to.be.revertedWith('ERC1155: insufficient balance for transfer') 

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

As seen from the first test, if the account firstAddress claims reward before the owner calls withdrawRemainingTokens, all will be good, and firstAddress will have 1 unit of reward token, and the contract owner will have 99 units.

But, if a contract owner calls withdrawRemainingTokens before the account firstAddress claim rewards, he will get all reward tokens (he will have 100 units), and on the other side, firstAddress will get error ERC1155: insufficient balance for transfer.

Tools Used

VSCode, Hardhat, Solidity Visual Developer

Recommended Mitigation Steps

In the ERC1155Quest.sol contract (function Erc1155Quest.sol#L54):

1. calculate how many unclaimed rewards uint unclaimedRewards = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
2. calculate how many are non-claimable rewards IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedRewards
3. transfer non-claimable rewards

function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint256 unclaimedRewards = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
        uint256 nonClaimableRewards =
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedRewards;
        IERC1155(rewardToken).safeTransferFrom(
            address(this), to_, rewardAmountInWeiOrTokenId, nonClaimableRewards, "0x00"
        );
}

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