code-423n4 / 2023-01-rabbithole-findings

1 stars 2 forks source link

Unlike `questFee_` , `royaltyFee_` is missing Upper Limit #651

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

In the QuestFactory.sol contract, as expected there is an upper limit on the questFee_ so that no unjust amount is set for the questFee_

186-189:     function setQuestFee(uint256 questFee_) public onlyOwner { 
        if (questFee_ > 10_000) revert QuestFeeTooHigh();  
        questFee = questFee_;
    }

However, in RabbitHoleReceipt.sol, the function setRoyaltyFee() does not set an upper limit to the royaltyFee_ parameter. Thus any arbitrarily large values can be set for royaltyFee_

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

90-93: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }

setting large values of royaltyFee_ will change all calculations in the contract wherever royaltyFee_ is involved and very large sums of money will flow towards royalty fees. Royalty fee is meant to be a small amount, however here that is not the case.

Recommended Mitigation Steps

modify the setRoyaltyFee() function to set an upper limit to the royaltyFee_ . A code similar to the following can be used

function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
        if (royaltyFee_ > 1_000) revert RoyaltyFeeTooHigh();  
        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }
c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

c4-sponsor commented 1 year ago

waynehoover marked the issue as sponsor confirmed

c4-judge commented 1 year ago

kirk-baird marked the issue as grade-a