code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The onlyOwner role has a single point of failure and onlyOwner can use critical a few functions.

onlyOwner role in the project: Owner is not behind a multisig

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.

onlyOwner functions;


contracts/QuestFactory.sol:
  142:     function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
  159:     function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
  165:     function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
  172:     function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
  179:     function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
  186:     function setQuestFee(uint256 questFee_) public onlyOwner {

This increases the risk of A single point of failure

Tools Used

Manuel Code Review

Recommended Mitigation Steps

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.

Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Also detail them in documentation and NatSpec comments