withdrawRemainingTokens fails to consider unclaimed receipts in Erc1155Quest

[code423n4]

Lines of code

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

Vulnerability details

The withdrawRemainingTokens implementation present in the Erc1155Quest allows the owner of the quest to claim back remaining tokens after the quest end time:

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

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

The function will transfer all of the rewards tokens present in the contract and does not consider potential receipts that are still pending to be claimed.

Impact

After the owner of the quest calls withdrawRemainingTokens, all unclaimed receipts will be rendered worthless, since the quest contract doesn't have any tokens to be handed as rewards.

If a user with an unclaimed receipt tries to call claim() to redeem their reward, the invocation will be reverted as the contract doesn't have the required tokens, which were previously withdrawn by the owner.

PoC

In the following test, Alice tries to claim her rewards after the owner has called withdrawRemainingTokens, which will fail due to insufficient funds in the contract.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function claimReceipt(address account, string memory questId) internal {
        (bytes32 hash, bytes memory signature) = signReceipt(account, questId);
        vm.prank(account);
        factory.mintReceipt(questId, hash, signature);
    }
    
    function test_Erc1155Quest_UnclaimedReceipts() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 10;
        uint256 tokenId = 0;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        Erc1155Quest quest = Erc1155Quest(
            factory.createQuest(
                address(tickets),
                endTime,
                startTime,
                totalParticipants,
                tokenId,
                "erc1155",
                questId
            )
        );

        vm.stopPrank();

        vm.prank(minter);
        tickets.mint(address(quest), tokenId, totalParticipants, "");

        vm.prank(deployer);
        quest.start();

        // Alice claims her receipt
        claimReceipt(alice, questId);

        // simulate time elapses until the end of the quest
        vm.warp(endTime);

        // owner withdraws tokens
        vm.prank(deployer);
        quest.withdrawRemainingTokens(deployer);

        // Alice tries to claim her reward, it will fail since withdrawRemainingTokens doesn't consider unclaimed receipts
        vm.expectRevert();
        vm.prank(alice);
        quest.claim();

        assertEq(tickets.balanceOf(alice, tokenId), 0);
        assertEq(tickets.balanceOf(deployer, tokenId), totalParticipants);
    }
}

Recommendation

Similar to the implementation of Erc20Quest, the contract can query the factory to know how many receipts are still pending to be claimed and withhold those funds in the contract for the users to claim.

contract Erc1155Quest is Quest, ERC1155Holder {
    ...
    QuestFactory public immutable questFactoryContract;
  
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint256 balance = IERC1155(rewardToken).balanceOf(address(this);
        uint256 unclaimed = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
        if (balance > unclaimed) {
          IERC1155(rewardToken).safeTransferFrom(
              address(this),
              to_,
              rewardAmountInWeiOrTokenId,
              balance - unclaimed,
              '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)