hash - Max curator fee would be bypassed for existing curators

[sherlock-admin3]

hash

medium

Max curator fee would be bypassed for existing curators

Summary

Max curator fee would be bypassed for existing curators

Vulnerability Detail

In the protocol, the admin can set the max fees for curation

    function setFee(Keycode auctionType_, FeeType type_, uint48 fee_) external override onlyOwner {
        // Check that the fee is a valid percentage
        if (fee_ > _FEE_DECIMALS) revert InvalidFee();

        // Set fee based on type
        // Or a combination of protocol and referrer fee since they are both in the quoteToken?
        if (type_ == FeeType.Protocol) {
            fees[auctionType_].protocol = fee_;
        } else if (type_ == FeeType.Referrer) {
            fees[auctionType_].referrer = fee_;
        } else if (type_ == FeeType.MaxCurator) {
            fees[auctionType_].maxCuratorFee = fee_;
        }

But even if the max curator fee is lowered, a curator can enjoy their old fees (which can be higher than the current max curator fees) since the curate function doesn't check for this condition

    function curate(uint96 lotId_, bytes calldata callbackData_) external nonReentrant {

        ....

        feeData.curated = true;
        feeData.curatorFee = fees[keycodeFromVeecode(routing.auctionReference)].curator[msg.sender];

Impact

Curators can obtain a fee higher than the max fee

Code Snippet

curate function doesn't check the max fee constraint https://github.com/sherlock-audit/2024-03-axis-finance/blob/cadf331f12b485bac184111cdc9ba1344d9fbf01/moonraker/src/AuctionHouse.sol#L652

Tool used

Manual Review

Recommendation

When fetching the fees inside the curate function, check if the fee is greater than the current max curator fee. If this is the case, set the fees to the max curator fee

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Axis-Fi/moonraker/pull/158

[AtanasDimulski]

Escalate,

In order for a curator to set himself as the curator and start receiving fees, he first has to call the setCuratorFee() function

    function setCuratorFee(Keycode auctionType_, uint48 fee_) external {
        // Check that the fee is less than the maximum
        if (fee_ > fees[auctionType_].maxCuratorFee) revert InvalidFee();

        // Set the fee for the sender
        fees[auctionType_].curator[msg.sender] = fee_;
    }

As shown in the above code snippet there is a check that the fee is not bigger than the maxCurator fee. Therefore a curator cannot simply bypass the max curator fee. In the case the protocol decides to lower the curator fee, and there is a curator that still hasn't set his fee, the curator will have to frontrun the transaction and set his fee with the higher maxCuratorFee, (or considering the possibility that the same address can be a curator for different auctions seems highly unlikely).

1. This should be considered an admin mistake and as per sherlock rules An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.
2. There is a certain trust assumption that the curator won't try and scam the auction creator, as curator address parameter is controlled by the auction creator - a case can be made that setting a curator who is willing to try and scam the auction creator is a user mistake and as per sherlock rules User input validation to prevent user mistakes is not considered a valid issue.

3. If the protocol decides to offer different curator fees, it can simply add a new module with the desired curator fee.

   This issue should be considered as low, as there is no direct loss of funds or breaking core contract functionality

[sherlock-admin2]

Escalate,

In order for a curator to set himself as the curator and start receiving fees, he first has to call the setCuratorFee() function

    function setCuratorFee(Keycode auctionType_, uint48 fee_) external {
        // Check that the fee is less than the maximum
        if (fee_ > fees[auctionType_].maxCuratorFee) revert InvalidFee();

        // Set the fee for the sender
        fees[auctionType_].curator[msg.sender] = fee_;
    }

As shown in the above code snippet there is a check that the fee is not bigger than the maxCurator fee. Therefore a curator cannot simply bypass the max curator fee. In the case the protocol decides to lower the curator fee, and there is a curator that still hasn't set his fee, the curator will have to frontrun the transaction and set his fee with the higher maxCuratorFee, (or considering the possibility that the same address can be a curator for different auctions seems highly unlikely).

1. This should be considered an admin mistake and as per sherlock rules An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.
2. There is a certain trust assumption that the curator won't try and scam the auction creator, as curator address parameter is controlled by the auction creator - a case can be made that setting a curator who is willing to try and scam the auction creator is a user mistake and as per sherlock rules User input validation to prevent user mistakes is not considered a valid issue.

3. If the protocol decides to offer different curator fees, it can simply add a new module with the desired curator fee.

   This issue should be considered as low, as there is no direct loss of funds or breaking core contract functionality

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Agree with @AtanasDimulski, I believe this issue can be considered low severity, given it could also be quite unfair for curators with higher than original max fees to suddenly have their fees subject to slashing

[Evert0x]

Planning to accept escalation and invalidate issue. I believe the following judging rule applies

An admin action can break certain assumptions about the functioning of the code.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• AtanasDimulski: accepted

[10xhash]

The protocol team fixed this issue in the following PRs/commits: Axis-Fi/moonraker#158

FIxed Now the fee is set to maxCuratorFee at the time of auction creation in case the curator's fee is greater

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.