RabbitHole Quest Protocol contest - oberon's results

Lines of code

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

Vulnerability details

Impact

Anyone can call the mint functions on line 83 & 97 and supply malicious arguments considering it's a public function and no adequate checks are in these blocks of code. The protocol is fundamentally broken due to the lack of access controls on these vital functions.

Proof of Concept

    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }

The onlyMinter() modifier does not produce the intended result of ensuring that only the minterAddress can supply arguments to the mint functions. Currently the modifier pushes the minterAddress into the stack with an SLOAD and then pushes minterAddress out of the stack 6 opcodes later without actually performing the check. You need to add the require statement to this modifier to prevent anyone from minting.

Tools Used

Manual review

Recommended Mitigation Steps

The require statement needs to be in the modifier for the revert to execute.

    modifier onlyMinter() {
        require(msg.sender == minterAddress);
        _;
    }

LOW

---

[L-01] MAXPROTOCOLREWARD() & PROTOCOLFEE() CAN ROUND DOWN TO 0

Context: Erc20Quest.sol:52 Erc20Quest.sol:96

    function maxTotalRewards() public view returns (uint256) {
        return totalParticipants * rewardAmountInWeiOrTokenId;
    }

    function maxProtocolReward() public view returns (uint256) {
        return (maxTotalRewards() * questFee) / 10_000;
    }

    function receiptRedeemers() public view returns (uint256) {
        return questFactoryContract.getNumberMinted(questId);
    }

    function protocolFee() public view returns (uint256) {
        return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    }

Description: If the totalParticipants, rewardAmountInWeiOrToken & questFee calculation comes out to below 10,000 all instances where the maxProtocolReward() is used will have faulty logic. The same problem is in protocolFee().

Recommendation: Add checks to ensure the the contract will only be constructed if reasonable minimum values for the amounts are given.

---

[L-02] LACK OF MSG.SENDER CHECKS

Context: RabbitHoleReceipt.sol:109

    function getOwnedTokenIdsOfQuest(
        string memory questId_,
        address claimingAddress_
    ) public view returns (uint[] memory) {
        uint msgSenderBalance = balanceOf(claimingAddress_);
        uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);

Description: Your contracts lack checks almost everywhere, but we'll look at this specific example. Here we have a function that is explicitly defining itself to be dealing with msg.sender logic by the verbiage msgSenderBalance yet anyone can insert a claiming address and get the balanceOf. The Quest.sol:99 may have the msg.sender, but that doesn't make the function safe due to its public declaration. This code block eventually leads to a _safeTransfer in Erc20Quest.sol.

Recommendation:

       + require(msg.sender == claimingAddress_);
       uint msgSenderBalance = balanceOf(claimingAddress_);

---

[L-03] MISSING ZERO ADDRESS CHECKS

Context: All contracts and functions fail to add this sanity check besides QuestFactory.sol:166

Description: Adding a zero address sanity check to your methods prevents potentially costly mistakes. Adding the check is an easy and effective way to reduce human error.

Recommendation:

    require([ADDR_ARGUMENT] != address(0));

---

[L-04] USE 2 STEP OWNABLE

Context: Quest.sol

Description: Using a 1 step ownable is not recommended and can cause serious issues if a mistake is made. It's standard practice to make use of a multi step ownable setup instead.

Recommendation: Ownable2Step.sol

---

QA

---

[NC-01] LACK OF EMIT EVENTS

Context: Quest.sol

Description: Incredibly important functions such as start(), pause() and unPause() lack emit events. When state changes these won't emit information that users and dApps desire.

    function pause() public onlyOwner onlyStarted {
        isPaused = true;
        emit Pause(isPaused);
    }

---

[NC-02] OUTDATED COMPILER

Solidity version 0.8.15 is missing features and bug features listed here.

---

[NC-03] SPELLING ERROR

QuestFactory.sol remave -> remove

---

[NC-04] OPEN TODO

IQuest:4

Please ensure that TODOs don't end up being deployed.