RabbitHoleReceipt's address might be changed therefore only manual mint will be available

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L13
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L44
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L95-L104
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L215-L229

Vulnerability details

Impact

Might be impossible to claim rewards by users. And admins must distribute tokens manually and pay fee for this. On a huge amount of participants this leads to huge amount of fees.

Proof of Concept

Let's consider QuestFactory. It has:

    RabbitHoleReceipt public rabbitholeReceiptContract;

Which responsible for mint tokens for users.

Then consider createQuest function. Here we pass rabbitholeReceiptContract into Quest.

In Quest this field is immutable.

Now lets consider next case:

1. We initialized whole contracts.
2. We created new Quest.
3. Next we decided to change rabbitholeReceiptContract in QuestFactory for another. To do this we call: setRabbitHoleReceiptContract. And successfully changing address.
4. Next we distribute signatures to our participants.
5. Users starts to mint tokens. But here is a bug QuestFactory storages new address of rabbitholeReceiptContract, but Quest initialized with older one. So users successfully minted their tokens, but can't exchange them for tokens because the Quest's receipt contract know nothing about minted tokens.

Possible solution here is change minterAddress in the original RabbitHoleReceipt contract and manually mint tokens by admin, but it will be too expensive and the company may lost a lot of money.

Tools Used

Manual audit

Recommended Mitigation Steps

In QuestFactory contract in the function mintReceipt the rabbitholeReceiptContract must be fetched from the quest directly.
To Quest Add:

function getRabbitholeReceiptContract() public view returns(RabbitHoleReceipt) {
    return rabbitHoleReceiptContract;
}

Modify mintReceipt function in QuestFactory like:

function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
    ...
    RabbitHoleReceipt rabbitholeReceiptContract = Quest(quests[questId_].questAddress).getRabbitholeReceiptContract();
    rabbitholeReceiptContract.mint(msg.sender, questId_);
    ...
}

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-sponsor]

waynehoover marked the issue as disagree with severity

[waynehoover]

Since our contract is upgradeable, they have to trust us that we aren’t going to do this during live quests. This was an emergency function, and likely won’t ever need to be used and only be accessible by only owner/us.

[kirk-baird]

This is a valid issue as upgrading the receipt contract will break currently open quests to prevent minting of receipts. This does not result in a loss of funds as they can be recovered by the quest creator.

Additionally, it is only accessible by the admin and so I'm going to downgrade this to a medium.

[c4-judge]

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

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as selected for report