RabbitHole Quest Protocol contest - 0xMirce's results

Lines of code

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

Vulnerability details

Impact

Without custom error or require statement at modifier onlyMinter, anyone could mint a receipt in the RabbitHoleReceipt contract or a ticket in the RabbitHoleTickets contract.

Proof of Concept

For RabbitHoleReceipt add a test in RabbitHoleReceipt.spec.ts

describe('additional mint test', () => {
        it('check onlyMinter modifier', async () => {
              await RHReceipt.connect(minterAddress).mint(firstAddress.address, 'def456')
              expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(1)

              await RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')
              expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(2)

              await RHReceipt.connect(royaltyRecipient).mint(firstAddress.address, 'def456')
              expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(3)

              expect(minterAddress.address != firstAddress.address && minterAddress.address != royaltyRecipient.address).to.equal(true)
         })
})

For RabbitHoleTickets add a test in RabbitHoleTickets.spec.ts

describe('additional mint test', () => {
        it('check onlyMinter modifier', async () => {
              await RHTickets.connect(minterAddress).mint(firstAddress.address, 1, 5, "0x")
              expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(5)

              await RHTickets.connect(firstAddress).mint(firstAddress.address, 1, 5, "0x")
              expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(10)

              await RHTickets.connect(royaltyRecipient).mint(firstAddress.address, 1, 5, "0x")
              expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(15)

              expect(minterAddress.address != firstAddress.address && minterAddress.address != royaltyRecipient.address).to.equal(true)
        })
})

As it is seen from these tests code, it is possible from anyone to call the mint operation in the RabbitHoleReceipt and RabbitHoleTickets (also, there is an additional except expect(minterAddress.address != firstAddress.address && minterAddress.address != royaltyRecipient.address).to.equal(true) which shows that addresses that call mint operation are different than minter address).

Tools Used

VSCode, Hardhat, Solidity Visual Developer plugin for VSCode

Recommended Mitigation Steps

Create and add a custom error in the if statement for a check is msg.sender different than minterAddress at lines RabbitHoleTickets.sol#L48 and RabbitHoleReceipt.sol#L59

modifier onlyMinter() {
       if (msg.sender != minterAddress) revert OnlyMinterAllowed();
       _;
}

Lines of code

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

Vulnerability details

Impact

When the quest ended, the ERC1155Quest contract owner could withdraw all remaining tokens, including users' reward tokens which are not claimed before the end. Because of that, if the user wants to claim their rewards after the quest end and after the ERC1155Quest contract owner calls a function withdrawRemainingTokens it will get an error that there are not enough tokens at the ERC1155Quest contract and not possible to claim a reward.

On the other side, in the ERC20Quest contract by lines Erc20Quest.sol#L84:L86 there is calculation how many tokens are not claimed, so the contract owner will get the difference between current contract balance and not claimed rewards. Because of that calculation, users could claim their reward after the ERC20Quest ended and the contract owner calls a function withdrawRemainingTokens.

Proof of Concept

Add two tests in Erc1155Quest.spec.ts

it('after quest end, should claim and withdraw remaining tokens', async () => {
    await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
    await deployedQuestContract.start()

    await ethers.provider.send('evm_increaseTime', [86400])

    expect(await deployedSampleErc1155Contract.balanceOf(firstAddress.address, rewardAmount)).to.equal(0)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

    await deployedQuestContract.connect(firstAddress).claim()

    await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

    expect(await deployedSampleErc1155Contract.balanceOf(firstAddress.address, rewardAmount)).to.equal(1)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(99)
    await ethers.provider.send('evm_increaseTime', [-86400])
})

it('after quest end, should withdraw remaining tokens and cannot claim', async () => {
    await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
    await deployedQuestContract.start()

    await ethers.provider.send('evm_increaseTime', [86400])

    expect(await deployedSampleErc1155Contract.balanceOf(firstAddress.address, rewardAmount)).to.equal(0)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

    await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)
    expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(100)
    await expect(
    deployedQuestContract.connect(firstAddress).claim()
    ).to.be.revertedWith('ERC1155: insufficient balance for transfer') 

    await ethers.provider.send('evm_increaseTime', [-86400])
})

As seen from the first test, if the account firstAddress claims reward before the owner calls withdrawRemainingTokens, all will be good, and firstAddress will have 1 unit of reward token, and the contract owner will have 99 units.

But, if a contract owner calls withdrawRemainingTokens before the account firstAddress claim rewards, he will get all reward tokens (he will have 100 units), and on the other side, firstAddress will get error ERC1155: insufficient balance for transfer.

Tools Used

VSCode, Hardhat, Solidity Visual Developer

Recommended Mitigation Steps

In the ERC1155Quest.sol contract (function Erc1155Quest.sol#L54):

1. calculate how many unclaimed rewards uint unclaimedRewards = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
2. calculate how many are non-claimable rewards IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedRewards
3. transfer non-claimable rewards

function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint256 unclaimedRewards = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
        uint256 nonClaimableRewards =
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedRewards;
        IERC1155(rewardToken).safeTransferFrom(
            address(this), to_, rewardAmountInWeiOrTokenId, nonClaimableRewards, "0x00"
        );
}

Optimize endTime and startTime checks

With a small refactor Quest.sol#L35:L37, one check could be deleted.

Instead of

	if (endTime_ <= block.timestamp) revert EndTimeInPast();
        if (startTime_ <= block.timestamp) revert StartTimeInPast();
        if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();

it could be used

        if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime()
        if (startTime_ <= block.timestamp) revert StartTimeInPast();

because if endTime is greater than startTime and startTime is greater than block.timestamp, automatically endTime will be greater than block.timestamp.

Recommended Mitigation Steps

Remove if (endTime_ <= block.timestamp) revert EndTimeInPast(); and use only

        if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime()
        if (startTime_ <= block.timestamp) revert StartTimeInPast();

TransferOwnership should be a two-step process

In all project contracts, it is using the OwnableUpgradeable contract. The main problem with that contract is that It’s possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. Instead of using the OwnableUpgradeable, my suggestion is to use the Ownable2StepUpgradeable contract, so when an account with the onlyOwner role transfer ownership to the other account, the other account (pendingOwner) must confirm ownership by calling acceptOwnership and then he will become the new owner. In that way, even if the account with the onlyOwner mistake when calling the transferOwnership transaction, it is possible to propose a new pending owner again and correct that mistake.

Recommended Mitigation Steps

Add Ownable2StepUpgradeable instead of OwnableUpgradeable in all project contracts.

Missing onlyNotStarted modifier in start() function

In line Quest.sol#L50 it is missing onlyNotStarted modifier. The reason why this modifier is missing is a case that this function has the same behavior as the unPause function when the quest starts (if the quest starts and the user calls the pause function, it is possible to unpause, and with start and with unpause functions).

Recommended Mitigation Steps

Create modifier onlyNotStarted and add to line Quest.sol#L50

modifier onlyNotStarted() {
        if (hasStarted) revert Started();
        _;
}

It is possible to grant or revoke CREATE_QUEST_ROLE role and with only owner and with DEFAULT_ADMIN_ROLE role

It is not so common that in the same contract exists Ownable and AccessControl access types. By default case in this contract, where onlyOwner and DEFAULT_ADMIN_ROLE are the same address, all will be without problems.

But, if the address for one of onlyOwner or DEFAULT_ADMIN_ROLE is changed, then it would be possible to call grantRole and revokeRole with the onlyOwner account with function from the line QuestFactory.sol#L142. In the other case, it will be possible with DEFAULT_ADMIN_ROLE directly call QuestFactoryContract.grantRole() or QuestFactoryContract.revokeRole() and this could cause potential divergence.

Recommended Mitigation Steps

Remove the function from the line QuestFactory.sol#L142