ERC1155: Admin can withdraw unclaimed token

[code423n4]

Lines of code

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

Vulnerability details

Impact

The ERC20 Quest ensures that unclaimed funds will be locked from Admin withdrawal even after endTime. This ensures User to claim there funds by redeeming ticket at any moment

The same check is missing in case of ERC1155 Quest. Admin will be able to withdraw token which user missed to claim within endTime which is wrong

Proof of Concept

1. Assume X users were minted ERC1155 tickets

function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
        _mint(to_, id_, amount_, data_);
    }

2. Lets say X-10 users have claimed there ticket and got required funds using the claim function

3. Now endTime has reached

4. Admin calls the withdrawRemainingTokens function to withdraw the excess 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'
        );
    }

5. Ideally 10 users are still remaining from claiming their tickets, so Owner should be allowed to only withdraw balance-10 amount

6. But seems like this check is missing and Admin can withdraw full balance

IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );

Recommended Mitigation Steps

Calculate the total number of participant of the ERC1155 quest by the endTime, lets say this number is X
Owner should now only be allowed to retrieve totalParticipants-X tokens

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-sponsor]

waynehoover marked the issue as disagree with severity

[waynehoover]

We don’t want the excess 1155 tokens, they should be locked in the Quest Contract so no users can ever get them

[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 marked issue #528 as primary and marked this issue as a duplicate of 528

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)