RabbitHole Quest Protocol contest - SovaSlava's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L83 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L97

Vulnerability details

Impact

Modifier onlyMinter miss require(). So code will always continue execution.

Proof of Concept

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

Recommended Mitigation Steps

modifier onlyMinter() {
require(msg.sender == minterAddress, "only minter");
_;
}

Modifier onlyAdminWithdrawAfterEnd() in Quest.sol dont have check of msg.sender. Advice to rename onlyAdminWithdrawAfterEnd to withdrawAfterEnd

    modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
//  NO CHECKS MSG.SENDER == OWNER
    }

its not critical, because receiver of fee is set in constructor

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L222

Vulnerability details

Impact

If project will be deployed to different blockchains, user can use one hash & signature for call function mintReceipt on one blockchain and after that on another blockchain. Because hash dont has chainId.

Recommended Mitigation Steps

 if (keccak256(abi.encodePacked(msg.sender, questId_, block.chainid)) != hash_) revert InvalidHash();