bin2chen - Liquidator can set up referrals for other users

[sherlock-admin4]

bin2chen

medium

Liquidator can set up referrals for other users

Summary

If a user has met the liquidation criteria and currently has no referrer then a malicious liquidator can specify a referrer in the liquidation order. making it impossible for subsequent users to set up the referrer they want.

Vulnerability Detail

Currently, there are 2 conditions to set up a referrer

1. the order cannot be empty （Non-empty orders require authorization unless they are liquidation orders）
2. there can't be another referrer already

    function _loadUpdateContext(
        Context memory context,
        address account,
        address referrer
    ) private view returns (UpdateContext memory updateContext) {
...
        updateContext.referrer = referrers[account][context.local.currentId];
        updateContext.referralFee = IMarketFactory(address(factory())).referralFee(referrer);
    }

    function _processReferrer(
        UpdateContext memory updateContext,
        Order memory newOrder,
        address referrer
    ) private pure {
@>      if (newOrder.makerReferral.isZero() && newOrder.takerReferral.isZero()) return;
        if (updateContext.referrer == address(0)) updateContext.referrer = referrer;
        if (updateContext.referrer == referrer) return;

        revert MarketInvalidReferrerError();
    }

    function _storeUpdateContext(Context memory context, UpdateContext memory updateContext, address account) private {
...
        referrers[account][context.local.currentId] = updateContext.referrer;
    }

However, if the user does not have a referrer, the liquidation order is able to meet both of these restrictions

This allows the liquidator to set up referrals for other users.

When the user subsequently tries to set up a referrer, it will fail.

Impact

If a user is set up as a referrer by a liquidated order in advance, the user cannot be set up as anyone else.

Code Snippet

https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L503

Tool used

Manual Review

Recommendation

Restrictions on Liquidation Orders Cannot Set a referrer

    function _processReferrer(
        UpdateContext memory updateContext,
        Order memory newOrder,
        address referrer
    ) private pure {
+       if (newOrder.protected() && referrer != address(0)) revert MarketInvalidReferrerError;
        if (newOrder.makerReferral.isZero() && newOrder.takerReferral.isZero()) return;
        if (updateContext.referrer == address(0)) updateContext.referrer = referrer;
        if (updateContext.referrer == referrer) return;

        revert MarketInvalidReferrerError();
    }

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, because this is the system design: referral can receive some fee from the user, and when liquidator liquidates - his (liquidator's) referral will get some fee from the user. Moreover, the referral is set for each order, and since no order can be placed until liquidation settles, the user can't execute orders for the same oracle version anyway, and for the following oracle versions a new referrer can easily be set

[nevillehuang]

@bin2chen66 This seems invalid based on @panprog comment.

[bin2chen66]

The system design doesn't seem to be like this. Could it be that I misunderstood? The referrer is set and cannot be modified.

1. _loadUpdateContext () take the previous referrer
2. context.local.currentId unchanged or + 1
3. _processReferrer () Verify that updateContext.referrer must be equal to the referrer of the current order
4. _storeUpdateContext () save referrers [account] [context.local.currentId]

The referrer inherits the previous one and verifies that it cannot be reset. @Panprog Can you see where can reset it, Did I miss it? Thanks.

[panprog]

@bin2chen66 @nevillehuang Yes, I agree with @bin2chen66 , the referrer can not be modified. Sorry for incorrect comment, I've missed that it's not reset between updates. I believe this is medium then as it allows to set the referral for the user during liquidation which he can't change.

[arjun-io]

It's accurate that the liquidator can set a referral address as part of the liquidation - this is acceptable. The referrer address is locked for the current ID but future orders (for a different local.currentId) should not have the referrer set - if they do that would be a bug. So as long as the liquidation version is filled, new orders for the next version should be fine.

[nevillehuang]

@arjun-io So do you agree this is a valid issue? I am incline to keep medium given referrer setting can be blocked

[arjun-io]

No I don't think it is, the only way this would be valid is if setting referrer is blocked for future orders that are not from the same Oracle Version. Would defer to auditors to double check this

[panprog]

@nevillehuang @arjun-io Yes, after the referrer is set once, user can never change it again, because it's loaded from currentId into updateContext, but then if local.currentId increases, the same referrer (from previous currentId) is stored into new currentId, thus referrer is always carried over from previous ids. So it seems that the issue which is valid is that it's impossible to change referrer once set, not that liquidator can set his referrer, although it wasn't really obvious from the docs (the intended functionality of setting the referrer).

[arjun-io]

Ah I see, the issue arises from the _storeUpdateContext using an updated context.local.currentId - this would also be an issue for liquidations then, I believe. In which case this is a deeper issue which might be a High

[panprog]

@arjun-io Do you mean that liquidator carries over from previous ids to currentId? Yes, it carries over like referrer, however, the accumulated liquidationFee will be 0 for all orders which are not protected (CheckPointLib._accumulateLiquidationFee):

    function _accumulateLiquidationFee(
        Order memory order,
        Version memory toVersion
    ) private pure returns (UFixed6 liquidationFee) {
        if (order.protected())
            return toVersion.liquidationFee.accumulated(Accumulator6(Fixed6Lib.ZERO), UFixed6Lib.ONE).abs();
    }

So there is no issue with liquidators. Even though the liquidators map will be set for all currentIds, this liquidator will be credited only once during the liquidation, and in following liquidations it will be overwritten with new liquidator. Yes, it's better to fix it so that liquidator doesn't carry over from previoud ids, but there is no impact in this right now.

And the impact for the issue described here I think is medium, not high.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/equilibria-xyz/perennial-v2/pull/297

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.