Skip to content

Latest commit

 

History

History
195 lines (157 loc) · 6.87 KB

File metadata and controls

195 lines (157 loc) · 6.87 KB

Protocol fee recipient address is copied into Erc20Quest contract

The QuestFactory contract defines an address which represents the recipient of the protocol fees for quests of type Erc20Quest. After a quest finishes, protocol fees can be sent to this address by calling the withdrawFee function.

The protocol fee recipient address is copied from the factory into the quest contract when a new quest is created using the createQuest function (line 84).

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

    ...
75  Erc20Quest newQuest = new Erc20Quest(
76      rewardTokenAddress_,
77      endTime_,
78      startTime_,
79      totalParticipants_,
80      rewardAmountOrTokenId_,
81      questId_,
82      address(rabbitholeReceiptContract),
83      questFee,
84      protocolFeeRecipient
85  );

If this address gets compromised or can't be accessed anymore, protocol owners can change it using the setProtocolFeeRecipient function:

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

function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
    if (protocolFeeRecipient_ == address(0)) revert AddressZeroNotAllowed();
    protocolFeeRecipient = protocolFeeRecipient_;
}

However, this will only update the reference in the factory contract. All existing quests will hold a reference to the previous address, which can't be updated.

Impact

Even though the protocol fee recipient address can be updated in the factory contract, all existing ERC20 quests must withdraw their fees to the previous address, as it cannot be updated in each quest contract individually.

If the protocol fee recipient account gets compromised or becomes inaccessible, protocol fees for each ongoing quest will be lost.

PoC

In the following test, the protocolFeeRecipient address is updated after the quest is created. Fees for that quest will be withdrawn to the previous address.

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

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

        // let's assume current protocolFeeRecipient gets compromised, owner changes it in the factory
        address compromisedProtocolFeeRecipient = protocolFeeRecipient;
        address newProtocolFeeRecipient = makeAddr("ProtocolFeeRecipient");
        vm.prank(deployer);
        factory.setProtocolFeeRecipient(newProtocolFeeRecipient);

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

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

        // Withdraw protocol fees
        uint256 protocolFee = quest.protocolFee();
        quest.withdrawFee();

        // Protocol fees are sent to the compromised address instead of the new one
        assertEq(token.balanceOf(newProtocolFeeRecipient), 0);
        assertEq(token.balanceOf(compromisedProtocolFeeRecipient), protocolFee);
    }
}

Recommendation

Instead of copying the address when the quest is deployed, the Erc20Quest contract can query the factory for the current value in the withdrawFee function:

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