ak1 - Global.sol#L47 : `incrementFees` is not deducting the `keeper fee` while calculating the `donation`.

[sherlock-admin]

ak1

high

Global.sol#L47 : incrementFees is not deducting the keeper fee while calculating the donation.

Summary

The function incrementFees updates the protocolFee , oracle fee, risk fee and donation amount.

The donation amount is the left over amount after deducting all the fee. But the incrementFees is not deducting the keeper fee before updating the donation amount.

Maket.sol uses this incrementFees function when processing the given global pending position into the latest position. Refer the line

Vulnerability Detail

As per document,

The pooled fees are split between the market and the protocol and market as follows:

protocol fee = total fees * protocolFee
market fee = total fees - protocol fee

(the protocolFee is a protocol-level parameter*)

The market fee is then further split as follows:

oracle fee = market fee * oracleFee + settlementFee
risk fee = market fee * riskFee

With the oracle fee going to the oracle factory of the market, and the risk fee being claimable by the risk coordinator.

Any leftover market fees are recorded as a donation and given to the beneficiary to support any miscellaneous additional fees.

so the donation amount is left over value after deducting all the fee amount.

when we look at the implementation,

    function incrementFees(
        Global memory self,
        UFixed6 amount,
        UFixed6 keeper,
        MarketParameter memory marketParameter,
        ProtocolParameter memory protocolParameter
    ) internal pure {
        UFixed6 protocolFeeAmount = amount.mul(protocolParameter.protocolFee);
        UFixed6 marketFeeAmount = amount.sub(protocolFeeAmount);

        UFixed6 oracleFeeAmount = marketFeeAmount.mul(marketParameter.oracleFee);
        UFixed6 riskFeeAmount = marketFeeAmount.mul(marketParameter.riskFee); -------->> (1) oracle fee updated without keeper fee.
        UFixed6 donationAmount = marketFeeAmount.sub(oracleFeeAmount).sub(riskFeeAmount); ----->> (2) donation amount is updated without keeper fee.

        self.protocolFee = self.protocolFee.add(protocolFeeAmount);
        self.oracleFee = self.oracleFee.add(keeper).add(oracleFeeAmount); ---->>> (3) keeper fee is included in oracle fee.
        self.riskFee = self.riskFee.add(riskFeeAmount);
        self.donation = self.donation.add(donationAmount);
    }

as shown in above code flow, the donation amount is calcualted without considering the keeper fee.

Impact

More donation amount is considered for beneficiary . Since it breaks one of the core feature of fee calculation, we are flagging this as High. for single call, this could be trivial, but when the volume will increase proportionally when number of calls increases, so the accumulated value will be high.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/Global.sol#L47-L65

Tool used

Manual Review

Recommendation

Update the incrementFee function as shown below.

    function incrementFees(
        Global memory self,
        UFixed6 amount,
        UFixed6 keeper,
        MarketParameter memory marketParameter,
        ProtocolParameter memory protocolParameter
    ) internal pure {
        UFixed6 protocolFeeAmount = amount.mul(protocolParameter.protocolFee);
        UFixed6 marketFeeAmount = amount.sub(protocolFeeAmount);

        UFixed6 oracleFeeAmount = marketFeeAmount.mul(marketParameter.oracleFee);
        UFixed6 riskFeeAmount = marketFeeAmount.mul(marketParameter.riskFee);
        UFixed6 donationAmount = marketFeeAmount.sub(oracleFeeAmount).sub(riskFeeAmount);

        self.protocolFee = self.protocolFee.add(protocolFeeAmount);
        self.oracleFee = self.oracleFee.add(keeper).add(oracleFeeAmount);
        self.riskFee = self.riskFee.add(riskFeeAmount);
        self.donation = self.donation.add(donationAmount); ------------------------>>> remove
        self.donation = self.donation.add(donationAmount).sub(keeper); ----------->>> Add
    }

[sherlock-admin]

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

141345 commented:

l

panprog commented:

invalid because amount is only fees, keeper fees are not included, they're in a keeper parameter