Liquidator/referrer is himself, rewards will be lost

Summary

The current protocol does not restrict the user from liquidating themselves or the referrer from setting it up as themselves If they are themselves, the current logic means that this part of the reward will be lost.

Vulnerability Detail

When execute _processOrderLocal() will give the liquidate fee and referral fee to the appropriate person.

    function _processOrderLocal(
        Context memory context,
        SettlementContext memory settlementContext,
        address account,
        uint256 newOrderId,
        Order memory newOrder
    ) private {
...
        _credit(liquidators[account][newOrderId], accumulationResult.liquidationFee);
        _credit(referrers[account][newOrderId], accumulationResult.subtractiveFee);

        emit AccountPositionProcessed(account, newOrderId, newOrder, accumulationResult);
    }

    function _credit(address account, UFixed6 amount) private {
        if (amount.isZero()) return;

@>      Local memory newLocal = _locals[account].read();
@>      newLocal.credit(amount);
        _locals[account].store(newLocal);
    }

    function credit(Local memory self, UFixed6 amount) internal pure {
        self.claimable = self.claimable.add(amount);
    }

The _credit() method reads claimable from storage and saves it to storage.

This will cause claimable to be overwritten if liquidators[account][newOrderId] or referrers[account][newOrderId] is the account of the current pendingOrder.

Assumptions: alice refers itself, or liquidates itself liquidators[account][orderId] = alice referrers[account][orderId] = alice

Executing settle(alice), would look like this:

settle() -> _loadContext()
- context.local.claimable =0
_processOrderLocal() -> _credit(alice,amount=10) -> write storage
- storage_local[alice].claimable=10
settle() -> _storeContext() -> _locals[account].store(context.local); ->context.local.claimable still 0
- storage_local[alice].claimable will be changed 0

Impact

The liquidator/referrer is himself, the reward will be lost and locked in the contract

Code Snippet

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

Tool used

Manual Review

Recommendation

    function _processOrderLocal(
        Context memory context,
        SettlementContext memory settlementContext,
        address account,
        uint256 newOrderId,
        Order memory newOrder
    ) private {
...
        context.local.update(newOrderId, accumulationResult);
        context.latestPosition.local.update(newOrder);

        _checkpoints[account][newOrder.timestamp].store(settlementContext.latestCheckpoint);

+       if (account == liquidators[account][newOrderId]) {
+             context.local.claimable +=accumulationResult.liquidationFee;
+        }else{
              _credit(liquidators[account][newOrderId], accumulationResult.liquidationFee);
+       }
+       if (account == referrers[account][newOrderId]) {
+          context.local.claimable +=accumulationResult.subtractiveFee;
+        }else{
             _credit(referrers[account][newOrderId], accumulationResult.subtractiveFee);
+       }
        emit AccountPositionProcessed(account, newOrderId, newOrder, accumulationResult);
    }

Duplicate of #16

sherlock-admin4 commented 5 months ago

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

panprog commented:

valid medium, dup of #16

takarez commented:

valid; medium(2)

sherlock-admin4 commented 4 months ago

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