Quest owner will receive less funds when withdrawing remaining tokens from the Erc20Quest if protocol fees are withdraw first

[code423n4]

Lines of code

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

Vulnerability details

After a Erc20Quest has ended, the owner of the quest can withdraw the remaining tokens from the contract by calling the withdrawRemainingTokens function:

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

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);

    uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
    uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
    IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
}

The calculation is done by fetching the current token balance of the contract and subtracting the protocol fees and the tokens corresponding to unclaimed receipts.

The contract also implements a separate function to withdraw protocol fees called withdrawFee:

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

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

If the withdrawFee function is called before calling withdrawRemainingTokens, then protocol fees will be accounted twice in the calculation. When withdrawFee is called it will transfer the funds to the protocol fee recipient, which means the balance of the contract will be reduced by that amount. But then withdrawRemainingTokens will use that balance and will also subtract again the protocol fee.

Impact

If the quest has protocol fees (i.e. protocolFee() > 0) and if the withdrawFee function is called before the owner calls withdrawRemainingTokens (assuming not all participants completed the quest), then the owner will receive less tokens than expected.

As discussed in the previous section, the implementation subtracts the protocol fee from the current token balance of the contract. If the fees have already been withdrawn, the token balance already reflects this, and subtracting the fees will result in the owner receiving fewer tokens than expected by the amount of protocolFee().

uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

Note that there's also the possibility of an arithmetic overflow. If the actual remaining tokens are less than the protocol fee, then the subtraction will cause an overflow due to the unsigned integer arithmetic.

PoC

In the following test, protocol fees are withdrawn before the owner calls withdrawRemainingTokens. The owner of the quest receives less than the expected amount of tokens.

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_WithdrawRemainingAfterFees() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 10;
        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);

        // protocol fees are withdrawn first...
        uint256 protocolFee = quest.protocolFee();
        quest.withdrawFee();
        assertEq(token.balanceOf(protocolFeeRecipient), protocolFee);

        // now the owner tries to withdraw the remaining tokens. Remaining tokens should equal:
        // total (rewards + fees) - alice reward - actual fee (fee for alice claim)
        uint256 expectedRemainingTokens = rewards +
            fees -
            1 *
            rewardAmountOrTokenId -
            protocolFee;
        vm.prank(deployer);
        quest.withdrawRemainingTokens(deployer);
        assertFalse(token.balanceOf(deployer) == expectedRemainingTokens);
        assertTrue(token.balanceOf(deployer) < expectedRemainingTokens);
    }
}

Recommendation

Add a flag to indicate if the protocol fees were already withdrawn and use this to check if the fees amount needs to be subtracted in the calculation for the withdrawRemainingTokens function.

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);

    uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
    uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - unclaimedTokens;
    if (!feesAlreadyWithdrawn) {
      nonClaimableTokens -= protocolFee();
    }
    IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
}

[c4-judge]

kirk-baird marked the issue as duplicate of #61

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

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