RabbitHole Quest Protocol contest - Garrett's results

Lines of code

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

Vulnerability details

The current implementation of the modifier onlyMinter() will not revert because the "require" part is missing, therefore any user will be able to access the minting functions in RabbitHoleTickets.sol and RabbitHoleReceipt.sol.

Impact

Any user than the allowed minter (minterAdress) is able to mint a receipt or ticket.

Proof of Concept

In RabbitHoleReceipt.spec.ts, on line 50, change the minterAddress with another address (for example to firstAddress):

await RHReceipt.connect(minterAddress).mint(firstAddress.address, 'def456')

With:

await RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')

Same test in RabbitHoleTickets.spec.ts on line 38 or 46.

Tools Used

Manual check

Recommended Mitigation Steps

Change to:

require(msg.sender == minterAddress, "not the allowed minter");

Lines of code

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

Vulnerability details

The function withdrawFee() does not account whether the fees have already been collected or not, therefore it can be called multiple times or even indefinitely, until the contract balance reaches zero. All funds will be transferred to the protocolFeeRecipient, but the participating users will not be able to get their reward. On a side note: the function use the onlyAdminWithdrawAfterEnd(), which only checks whether the endTime has been reached and there is no check on which user do the call, not sure if this is done by design. Anyway this mean that any user can call withdrawFee().

Impact

Contract can be drained (although the funds will go to protocolFeeRecipient), possibly denying the rewards or breaking the contract accounting.

Proof of Concept

In Erc20Quest.spec, copy and paste line 365 multiple times, for example:

await ethers.provider.send('evm_increaseTime', [100001])
await deployedQuestContract.withdrawFee()
await deployedQuestContract.withdrawFee() 
await deployedQuestContract.withdrawFee() 

You can see the call with go through and the contract balance will be less than expected

Tools Used

Manual check

Recommended Mitigation Steps

Make withdrawFee() callable only once