Users are paying twice the fee even if the two transactions belong to the same version

Summary

Users are charged double fees when they update their position twice in the same version.

Vulnerability Detail

In the _update function, a new position with an incremental id is created when the current timestamp is greater than the timestamp of the current position. All the fees are reset in the prepare function.

    // advance to next id if applicable
    if (context.currentTimestamp > context.currentPosition.local.timestamp) {
        context.local.currentId++;
        context.currentPosition.local.prepare();
    }
    if (context.currentTimestamp > context.currentPosition.global.timestamp) {
        context.global.currentId++;
        context.currentPosition.global.prepare();
    }

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L256-L264

    function prepare(Position memory self) internal pure {
        self.fee = UFixed6Lib.ZERO;
        self.keeper = UFixed6Lib.ZERO;
        self.collateral = Fixed6Lib.ZERO;
    }

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/Position.sol#L123-L129

However, when a user calls update twice in the same version to update their position, the fees are calculated again and accumulated from the previous pending position. The fee for the second update transaction is calculated by comparing with the previous pending position, which is the first update transaction in the same version.

    Order memory newOrder =
        context.currentPosition.local.update(context.currentTimestamp, newMaker, newLong, newShort); // compare with the previous pending position
    context.currentPosition.global.update(context.currentTimestamp, newOrder, context.riskParameter);

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L266-L274

All the keeper fees and fees from delta attributes are calculated and accumulated again.

In this case, the second update transaction only modifies a pending position, therefore the fees should not be calculated as the sum of both transactions. The fees should only be calculated using the net effect of two transactions.

Impact

Users are charged twice the fees even if the two transactions belong to the same version and not yet settled.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L256-L264 https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/Position.sol#L123-L129

Tool used

Manual Review

Recommendation

If two transactions belong to the same version, the fees should only be calculated using the net effect of two transactions.

sherlock-audit / 2023-07-perennial-judging

ast3ros - Users are paying twice the fee even if the two transactions belong to the same version #128