RabbitHole Quest Protocol contest - amaechieth'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/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98-L104 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L83-L85

Vulnerability details

Impact

The onlyMinter modifier is meant to act as access control, preventing unauthorised users from minting, however, the implementation of the modifier is missing a require/revert statement and is therefore implemented incorrectly. As this modifier is broken, it has full access to the mint function, which essentially breaks the functionality of this protocol.

Proof of Concept

This is the modifier in question.

modifier onlyMinter() {
        msg.sender == minterAddress; // @audit incorrect implementation
        _;
    }

This is a POC to show how it doesn't work, using the protocols existing tests & changing the caller of the function which is meant to be access restricted:

describe('mint', () => {
    it.only('mints a token with correct questId', async () => {
      await RHReceipt.connect(hackerAddress).mint(firstAddress.address, 'def456')

      expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(1)
      expect(await RHReceipt.questIdForTokenId(1)).to.eq('def456')
    })
  })

This is the output from running that test:

  RabbitholeReceipt Contract
    mint
      ✓ mints a token with correct questId

Tools Used

Manual Audit + Hardhat tests

Recommended Mitigation Steps

Change modifier to:

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