Broken modifier implementation allows anyone to call mint* functions

[code423n4]

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/RabbitHoleTickets.sol#L47-L50

Vulnerability details

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

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

Description

A mis-implementation of a modifier allows anyone to call the mint* functions of the RabbitHoleReceipt and RabbitHoleTickets contracts.

In the case of RabbitHoleReceipt.mint(), this allows an attacker to create valid Receipts for a Quest via its id, and then redeem the receipts for rewards via Erc20Quest.claim() and Erc1155Quest.claim(), draining all rewards to themselves.

In the case of RabbitHoleTickets, whatever benefit/value is applied to that token would be freely available to the minter.

PoC

Modify the existing RabbitHoleReceipt.spec.ts test 'mint' like so:

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

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

    // connect to the Receipt with non-privileged user, call mint
    await RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')
    // since there's no revert, we want their token balance to remain 1
    expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(1)
  })
})

The test will fail:

mints a token with correct questId:

AssertionError: expected 2 to equal 1. The numerical values of the given "ethers.BigNumber" and "number" inputs were compared, and they differed.
  + expected - actual

  -2
  +1

Mitigation

Fix the modifier as follows:

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

Note that this breaks other tests in the suite because of the use of owner.address instead of the QuestFactory contract in the initializer used to deploy the RabbitHoleReceipt.

Tools used

Provided test suite, manual review

[c4-judge]

kirk-baird marked the issue as duplicate of #9

[c4-judge]

kirk-baird marked the issue as satisfactory