Changing the RabbitHoleReceipt contract in the QuestFactory will break rewards for existing quests

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L172-L174

Vulnerability details

The RabbitHoleReceipt is a ERC721 token that represents a completed task for a specific quest and can be used to claim rewards. This token is minted in the QuestFactory contract using the mintReceipt function and rewards can be claimed using the claim function present in the Erc20Quest and Erc1155Quest contracts.

The QuestFactory contract contains a reference to the RabbitHoleReceipt in the rabbitholeReceiptContract variable. This address is forwarded to the quests contracts in the createQuest function, in line 82 for the Erc20Quest contract and line 115 for the Erc1155Quest contract. These contracts hold an immutable reference to the RabbitHoleReceipt (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L13).

The factory contract contains also a function to update the rabbitholeReceiptContract variable:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L172-L174

function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
    rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);
}

Now, if the rabbitholeReceiptContract variable is updated in the factory, current ongoing quests will be out of sync with respect to this reference, as these contracts contain an immutable reference to the previous rabbitholeReceiptContract value that was copied when they were created.

Receipt minting will be done using the new contract (mintReceipt in QuestFactory), while claiming will be done using the previous contract (claim in Erc20Quest or Erc1155Quest). Users that complete tasks after the RabbitHoleReceipt contract is updated will be minted the new receipt, which will fail to be claimed in the quest as these two are essentially different contracts.

Impact

After the RabbitHoleReceipt contract is updated in the QuestFactory contract, users that complete tasks for quests that were created before the receipt contract was updated won't be able to claim their rewards.

Given this scenario, users will mint their receipts using the mintReceipt function present in the QuestFactory contract which will use the new updated contract. However, if they attempt to claim their rewards using the claim function in the quest contract, the call will be reverted as the receipt contract here is outdated and their receipt will not be recognized.

PoC

In the following test, the RabbitHoleReceipt contract is updated in the factory after the quest is created. Alice mints her receipt after completing the quest, which fails to be claimed with the NoTokensToClaim() error, as the receipt she has is a different contract than the one the quest is expecting.

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

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

        factory.setQuestFee(0);

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

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        deal(address(token), address(quest), rewards);
        quest.start();

        vm.stopPrank();

        // Assume RabbitHoleReceiptContract is changed in the factory
        vm.startPrank(deployer);

        // deploy a new RabbitHoleReceipt
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            address(factory),
            0
        );

        // update the factory
        factory.setRabbitHoleReceiptContract(address(receipt));

        vm.stopPrank();

        vm.warp(startTime);

        // Claim receipt for Alice, this will use the new Receipt contract
        claimReceipt(alice, questId);

        // Now Alice tries to claim her rewards in the quest, the following will fail since
        // the quest is using the old Receipt contract
        vm.expectRevert(bytes4(keccak256("NoTokensToClaim()")));
        vm.prank(alice);
        quest.claim();

        assertEq(token.balanceOf(alice), 0);
    }
}

Recommendation

The straightforward solution would be to remove the mutation around the RabbitHoleReceipt reference in the QuestFactory contract. The RabbitHoleReceipt contract is upgradeable, which means it can be updated while keeping the same address. This will ensure that both the factory and the quests contract maintain the same reference to the receipt contract.

A bit more complicated solution would be to move the minting (mintReceipt) to the quest contract itself. This way both minting and claiming happens in the quest contract, which will always resolve to the same receipt contract. Note that this will require setting up all quest contracts as minters of the receipt, which could carry other potential risks.

[c4-judge]

kirk-baird marked the issue as duplicate of #425

[c4-judge]

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

[c4-judge]

kirk-baird marked the issue as satisfactory