RabbitHole Quest Protocol contest - usmannk's results

Lines of code

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

Vulnerability details

Impact

Anyone may mint rabbit hole receipts. Because the quest id is specified by the minter, this means that any user may drain all quests of their rewards via infinite mint.

Proof of Concept

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

The onlyMinter modifier does not have any require statements. As is, the msg.sender == minterAddress; check is a no-op.

Tools Used

Manual review

Recommended Mitigation Steps

Change msg.sender == minterAddress; to require(msg.sender == minterAddress);

To prevent regression, I recommend adding the following test to test/RabbitHoleReceipt.spec.ts

describe('onlyMinterCanMint', () => {
  it('reverts when mint is called without minter role', async () => {
    await expect(
      RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')
    ).to.be.reverted
  })
})

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76-L79

Vulnerability details

Impact

After the specified deadline, anyone may call functions with the onlyAdminWithdrawAfterEnd modifier, even if they are not an admin. This allows any user to call admin-only functions.

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76-L79

The onlyAdminWithdrawAfterEnd modifier performs no checks on msg.sender, despite being labeled onlyAdmin.

There are cases already where the onlyAdminWithdrawAfterEnd modifier is used on its own, without onlyOwner, due to the naming implying an admin check. For example: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Tools Used

Manual review.

Recommended Mitigation Steps

Add a check for the owner role in onlyAdminWithdrawAfterEnd.

Lines of code

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

Vulnerability details

Impact

Admins may withdraw tokens from quests even though there are remaining unclaimed receipts. This leads to a loss of funds for the receipt holders.

Proof of Concept

In Erc20Quest, there are checks that ensure that even on admin withdrawal, sufficient funds remain to cover already-minted but yet-unclaimed receipts: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L78-L87

However, in Erc1155Quest there are no such checks: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63

For Erc1155Quests, admin withdrawal will cause loss of funds for holders of minted but unclaimed receipts. This seems to be an error as there are checks in Erc20Quest that guard against this.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the admin withdrawal amount in Erc1155Quest such on withdrawal, (protocolFees + unclaimedTokenRewards) num of rewardTokens are left behind for currently minted but unclaimed receipts.