KingNFT - Keepers will suffer significant losses due to miss compensation for L1 rollup fees

[sherlock-admin]

KingNFT

high

Keepers will suffer significant losses due to miss compensation for L1 rollup fees

Summary

While keepers submits transactions to L2 EVM chains, they need to pay both L2 execution fee and L1 rollup fee. Actually, L1 fees are much higher than L2 fees. In many case, L2 fees can be practically negligible. The current implementation only compensate and incentive keepers based on L2 gas consumption, keepers will suffer significant losses.

Vulnerability Detail

As shown of keep() modifier (L40-57), only L2 execution fee are compensated.

File: perennial-v2\packages\perennial-oracle\contracts\pyth\PythOracle.sol
124:     function commitRequested(uint256 versionIndex, bytes calldata updateData)
125:         public
126:         payable
127:         keep(KEEPER_REWARD_PREMIUM, KEEPER_BUFFER, "")
128:     {
...
157:     }

File: perennial-v2\packages\perennial-extensions\contracts\MultiInvoker.sol
359:     function _executeOrder(
360:         address account,
361:         IMarket market,
362:         uint256 nonce
363:     ) internal keep (
364:         UFixed18Lib.from(keeperMultiplier),
365:         GAS_BUFFER,
366:         abi.encode(account, market, orders(account, market, nonce).fee)
367:     ) {
...
384:     }


File: root\contracts\attribute\Kept.sol
40:     modifier keep(UFixed18 multiplier, uint256 buffer, bytes memory data) {
41:         uint256 startGas = gasleft();
42: 
43:         _;
44: 
45:         uint256 gasUsed = startGas - gasleft();
46:         UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
47:             .mul(multiplier)
48:             .add(UFixed18Lib.from(buffer))
49:             .mul(_etherPrice())
50:             .mul(UFixed18.wrap(block.basefee));
51: 
52:         _raiseKeeperFee(keeperFee, data);
53: 
54:         keeperToken().push(msg.sender, keeperFee);
55: 
56:         emit KeeperCall(msg.sender, gasUsed, multiplier, buffer, keeperFee);
57:     }

Takes a random selected transaction at the writing time
https://optimistic.etherscan.io/tx/0xbb8e68e21c92acf4171fb6041b758b55acc3c559ec4595ed1129d534d90de995

We can find the L1 fee is much expensive than L2 fee.

L2 fee = 0.06 Gwei * 113,449 ~= 6,806 Gwei
L1 fee = 0.00006565634612417 ETH ~= 65656 Gwei
L1 / L2 = 964%

Typically, L1 fees are dynamic and determined by the calldata length and smoothed Ethereum gas prices. To submit Pyth oracle price, the VAA calldata will be 1700+ bytes, keepers need to pay much L1 rollup fee.

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/test/integration/pyth/PythOracle.test.ts#L34

Impact

No enough incentive for keeper to submit oracle price and execute orders, the system will not work.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/attribute/Kept.sol#L40

Tool used

Manual Review

Recommendation

Compensating L1 rollup fee, here are some reference:
https://docs.arbitrum.io/arbos/l1-pricing
https://community.optimism.io/docs/developers/build/transaction-fees/#the-l1-data-fee

[sherlock-admin]

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

141345 commented:

m

[arjun-io]

The Kept helper supports a buffer amount which can be used for this use case.

[ydspa]

Escalate

Using Kept buffer can't solve this problem, as the ratio of L1GasPrice / L2GasPrice changes in every block and can vary with a extreme wide range.
Let's say

CurrentL1GasPrice = 20 Gwei // it refers to smoothed recent Ethereum gas price provided by L2 system contract
CurretnL2GasPrice = 2 Gwei   // it refers to block.basefee
L1RollupGas = 40,000
L1RollupFee = L1RollupGas * CurrentL1GasPrice  = 40,000 * 20 = 800,000 Gwei

To compensate L1 rollup fee, we should set

buffer = L1RollupFee / CurrentL2GasPrice = 800,000 / 2 = 400,000

to make

L1Compensation = buffer *  CurrentL2GasPrice = 400,000 * 2 = 800,000 = L1RollupFee 

some time later, i.e. 1 hour, gas prices change to

NewL1GasPrice = 40 Gwei
NewL2GasPrice = 0.01 Gwei 

we can see the L1Compensation will be far less than L1RollupFee, keepers suffer losses

L1RollupFee = L1RollupGas * CurrentL1GasPrice = 40, 000 * 40 = 1,600,000 Gwei
L1Compensation = buffer * CurretnL2GasPrice = 400, 000 * 0.01 = 4,000 Gwei

In contrast, if gas prices change to

NewL1GasPrice = 10 Gwei
NewL2GasPrice = 2 Gwei

then, the L1Compensation is larger than L1RollupFee, keepers earn extra profit.

L1RollupFee = L1RollupGas * CurrentL1GasPrice = 40,000 * 10 = 400,000 Gwei
L1Compensation= buffer * CurretnL2GasPrice = 400,000 * 2 = 800,000 Gwei

Therefore, regardless of how we set buffer, as it's a fixed value, sometimes keepers suffer losses, and other times keepers earn extra profit.

During periods that keepers suffer losses, they are likely to stop working and the system is blocked.
And other periods , the protocol suffer losses.

About the severity:
(1) As the users' orders will be held until keepers become profitable, the waiting time may be short as some minutes, may be long as some hours, or even some days. Users' financial losses are foreseeable.
(2) while keepers earn extra profit, proper ratio of L1GasPrice / L2GasPrice and block.basefee could make keeperReward > settlementFee, then a new attack vector become feasible，keepers can set 1 wei orders by self to drain fund from protocol, as self.fee ~= 0, profit ~= keeperReward - self.keeper = keeperReward - settlementFee

File: perennial-v2\packages\perennial\contracts\types\Order.sol
50:     function registerFee(
51:         Order memory self,
52:         OracleVersion memory latestVersion,
53:         MarketParameter memory marketParameter,
54:         RiskParameter memory riskParameter
55:     ) internal pure {
...
64:         self.fee = self.maker.abs().mul(latestVersion.price.abs()).mul(UFixed6Lib.from(makerFee))
65:             .add(self.long.abs().add(self.short.abs()).mul(latestVersion.price.abs()).mul(UFixed6Lib.from(takerFee)));
66: 
67:         self.keeper = isEmpty(self) ? UFixed6Lib.ZERO : marketParameter.settlementFee;
68:     }

To sum up, I think it's high, not medium

[sherlock-admin2]

Escalate

Using Kept buffer can't solve this problem, as the ratio of L1GasPrice / L2GasPrice changes in every block and can vary with a extreme wide range.
Let's say

CurrentL1GasPrice = 20 Gwei // it refers to smoothed recent Ethereum gas price provided by L2 system contract
CurretnL2GasPrice = 2 Gwei   // it refers to block.basefee
L1RollupGas = 40,000
L1RollupFee = L1RollupGas * CurrentL1GasPrice  = 40,000 * 20 = 800,000 Gwei

To compensate L1 rollup fee, we should set

buffer = L1RollupFee / CurrentL2GasPrice = 800,000 / 2 = 400,000

to make

L1Compensation = buffer *  CurrentL2GasPrice = 400,000 * 2 = 800,000 = L1RollupFee 

some time later, i.e. 1 hour, gas prices change to

NewL1GasPrice = 40 Gwei
NewL2GasPrice = 0.01 Gwei 

we can see the L1Compensation will be far less than L1RollupFee, keepers suffer losses

L1RollupFee = L1RollupGas * CurrentL1GasPrice = 40, 000 * 40 = 1,600,000 Gwei
L1Compensation = buffer * CurretnL2GasPrice = 400, 000 * 0.01 = 4,000 Gwei

In contrast, if gas prices change to

NewL1GasPrice = 10 Gwei
NewL2GasPrice = 2 Gwei

then, the L1Compensation is larger than L1RollupFee, keepers earn extra profit.

L1RollupFee = L1RollupGas * CurrentL1GasPrice = 40,000 * 10 = 400,000 Gwei
L1Compensation= buffer * CurretnL2GasPrice = 400,000 * 2 = 800,000 Gwei

Therefore, regardless of how we set buffer, as it's a fixed value, sometimes keepers suffer losses, and other times keepers earn extra profit.

During periods that keepers suffer losses, they are likely to stop working and the system is blocked.
And other periods , the protocol suffer losses.

About the severity:
(1) As the users' orders will be held until keepers become profitable, the waiting time may be short as some minutes, may be long as some hours, or even some days. Users' financial losses are foreseeable.
(2) while keepers earn extra profit, proper ratio of L1GasPrice / L2GasPrice and block.basefee could make keeperReward > settlementFee, then a new attack vector become feasible，keepers can set 1 wei orders by self to drain fund from protocol, as self.fee ~= 0, profit ~= keeperReward - self.keeper = keeperReward - settlementFee

File: perennial-v2\packages\perennial\contracts\types\Order.sol
50:     function registerFee(
51:         Order memory self,
52:         OracleVersion memory latestVersion,
53:         MarketParameter memory marketParameter,
54:         RiskParameter memory riskParameter
55:     ) internal pure {
...
64:         self.fee = self.maker.abs().mul(latestVersion.price.abs()).mul(UFixed6Lib.from(makerFee))
65:             .add(self.long.abs().add(self.short.abs()).mul(latestVersion.price.abs()).mul(UFixed6Lib.from(takerFee)));
66: 
67:         self.keeper = isEmpty(self) ? UFixed6Lib.ZERO : marketParameter.settlementFee;
68:     }

To sum up, I think it's high, not medium

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.

[arjun-io]

Thanks for the thorough explanation on why a buffer won't work. We agree that we should modify the Kept function to estimate L1 gas costs, thanks for the report!

[141345]

The severity of medium seems more appropriate.

Because according to sherlock's HM criteria:

Medium:
viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.
The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future.

High:
This vulnerability would result in a material loss of funds, and the cost of the attack is low.

Here the imbalanced L1/L2 gas fee would not be too frequent. Most of the time, the buffer will be good to compensate for the keeper. The described loss will incur when all the special conditions are met, and the fixed buffer is not enough to cover the keeper's cost.

[arjun-io]

Fixed
Update Kept for L2s: equilibria-xyz/root#74 and equilibria-xyz/root#76
Update Pyth Oracle for L2: equilibria-xyz/perennial-v2#85

[hrishibhat]

@ydspa tend to agree with this comment:
#91 (comment)
A valid medium

[ydspa]

@ydspa tend to agree with this comment: #91 (comment) A valid medium

I think the conclusion of Here the imbalanced L1/L2 gas fee would not be too frequent is not correct, the ratio of L1GasPrice / L2GasPrice vary frequently in a very wide range. The following table shows 3 typical 2-hours range of Ethereum VS Optimistic gas price in the past two weeks. We can see the Ratio can vary from e1 to as large as e9.

DateTime	L1GasPrice	L2GasPrice(Base)	Ratio(L1/L2)	Link
2023-08-27 02:00	13.19 Gwei	55 wei	2.40e8	link
2023-08-27 02:30	16.20 Gwei	56 wei	2.30e8	link
2023-08-27 03:00	14.95 Gwei	58 wei	2.58e8	link
2023-08-27 03:30	15.55 Gwei	53 wei	2.93e8	link
2023-08-27 04:00	21.53 Gwei	51 wei	4.06e8	link
2023-08-22 02:00	46.41 Gwei	66 wei	0.7e9	link
2023-08-22 02:30	121.65 Gwei	53 wei	2.30e9	link
2023-08-22 03:00	89.76 Gwei	52 wei	1.73e9	link
2023-08-22 03:30	68.45 Gwei	51 wei	1.34e9	link
2023-08-22 04:00	42.59 Gwei	100 wei	0.43e9	link
2023-08-17 10:00	229.22 Gwei	3.36 Gwei	6.82e1	link
2023-08-17 10:30	121.38 Gwei	0.30 Gwei	4.05e2	link
2023-08-17 11:00	138.89 Gwei	1.64 Gwei	8.47e1	link
2023-08-17 11:30	61.60 Gwei	0.1 Gwei	6.16e2	link
2023-08-17 12:00	36.84 Gwei	0.0058 Gwei	6.35e3	link

It will cause users' orders been held up to some hours in 2 ways:
Let's say we set buffer according Ratio = 2e8 and plus up to 100% intended reward for keepers, which means the system can only work well while Ratio is during (2e8, 4e8).
So, in 2023-08-27 02:00 ~ 04:00 the system works, but in most cases, the system doesn't work such as
(1) in 2023-08-22 02:00 ~ 04:00, the Ratio is too high, keepers would suffer loss which might lead them to stop pushing price and users' orders are held.
(2) in 2023-08-17 10:00 ~ 12:00, the Ratio is too low, keepers earn too much reward, which will trigger the following protection, then no new price can be pushed to chain, all users' orders are held entirely.

File: packages\perennial-oracle\contracts\OracleFactory.sol
93:     function claim(UFixed6 amount) external {
94:         if (amount.gt(maxClaim)) revert OracleFactoryClaimTooLargeError();
...
97:     }

To be emphasized:

1. Regardless of how we set buffer, it can only work in a extreme narrow Ratio range as compared to (e1, e9).
2. Just in the past 2 weeks, we can easily find more examples like the above ones that would block users' orders for up to 2 hours. It's almost 100% sure this bug would be repeatedly triggered during the long lifetime of the protocol.
3. As a perpetual exchange APP, users' orders are held up to some hours, especially in the above case (2), the service is stopped entirely, users' financial losses are foreseeable, the impact is high.

Hence, the issue is with both high probability of occurrence and high impact, undoubtedly should be a high issue.

[141345]

Yes the fee ratio could vary across a wide span. Extreme high L1 gas fee is intermittent, and will repeat.
However, the protocol can set some fix buffer at 90% percentile of the L1/L2 fee ratio (maybe at e8 level), then the loss will only emerge in some certain scenarios.

Still seems medium due to conditional loss.

[ydspa]

Yes the fee ratio could vary across a wide span. Extreme high L1 gas fee is intermittent, and will repeat. However, the protocol can set some fix buffer at 90% percentile of the L1/L2 fee ratio (maybe at e8 level), then the loss will only emerge in some certain scenarios.

Still seems medium due to conditional loss.

(1）I'm convinced that you can't find a buffer that works at 90% percentile, it is even hard to work at 50%, you can set buffer with a fixed Ratio plus some intended reward such as 100% (in perennial testcase, it's only 50%), then verify data in the past 1 year

(2) conditional loss of 1 % probability VS 99% probability are not the same thing, i think the later should be treated as unconditional .

[141345]

Even the distribution is Pareto distribution (power law), percentile still works. 50%, 90% can be found.

[ydspa]

Even the distribution is Pareto distribution (power law), percentile still works. 50%, 90% can be found.

Give your specific parameters please

[141345]

you can't find a buffer that works at 90% percentile, it is even hard to work at 50%

percentile always can be found, such as medium number(50%).

On the opposite, mean value could be hard to find, those extreme high gas fee can make mean not converge.

[ydspa]

More proof: if we set buffer according e8 level as recommended above, then we can find lots of examples the actual Ratio falls below e6 which keepers would earn too much rewards (10,000% level) to trigger system protection to block price submission.

The instances are all from the past one month, and sample interval is 1 hour, that's mean, averagely speaking, the system will be blocked for about 127 hours (5+ days) in one month if we set buffer according e8 level.

index  blockHeight
0 108995426
1 108952226
2 108865826
3 108864026
4 108862226
5 108860426
6 108822626
7 108820826
8 108819026
9 108817226
10 108815426
11 108813626
12 108811826
13 108810026
14 108808226
15 108806426
16 108804626
17 108802826
18 108801026
19 108799226
20 108604826
21 108570626
22 108504026
23 108471626
24 108376226
25 108360026
26 108358226
27 108356426
28 108345626
29 108318626
30 108311426
31 108309626
32 108298826
33 108297026
34 108295226
35 108293426
36 108291626
37 108289826
38 108288026
39 108286226
40 108284426
41 108268226
42 108266426
43 108264626
44 108262826
45 108261026
46 108259226
47 108257426
48 108255626
49 108253826
50 108252026
51 108250226
52 108248426
53 108246626
54 108244826
55 108243026
56 108241226
57 108239426
58 108237626
59 108235826
60 108234026
61 108232226
62 108230426
63 108223226
64 108221426
65 108219626
66 108217826
67 108216026
68 108214226
69 108212426
70 108210626
71 108208826
72 108207026
73 108205226
74 108203426
75 108201626
76 108079226
77 107958626
78 107953226
79 107951426
80 107949626
81 107947826
82 107946026
83 107944226
84 107942426
85 107940626
86 107938826
87 107937026
88 107935226
89 107933426
90 107931626
91 107929826
92 107928026
93 107926226
94 107924426
95 107922626
96 107920826
97 107919026
98 107917226
99 107915426
100 107913626
101 107911826
102 107910026
103 107908226
104 107906426
105 107904626
106 107902826
107 107901026
108 107870426
109 107780426
110 107778626
111 107776826
112 107775026
113 107773226
114 107771426
115 107769626
116 107767826
117 107742626
118 107740826
119 107739026
120 107737226
121 107735426
122 107733626
123 107731826
124 107730026
125 107728226
126 107726426

[141345]

The main point is, there is some parameter, considering the trade off, can balance the loss and overpayment. e8 is just an example.

[ydspa]

The main point is, there is some parameter, considering the trade off, can balance the loss and overpayment. e8 is just an example.

What my meaning is the parameter doesn't exist at all, mathematically speaking, L1Price and L2Price are enough independent, we can't calculate the following result by only L2Price

Optimism Transaction Fee = [ Fee Scalar * L1 Gas Price * (Calldata + Fixed Overhead) ] + [ L2 Gas Price * L2 Gas Used ]

reference: https://dune.com/haddis3/optimism-fee-calculator

This is why no matter we set buffer according e1, e2, ..., e9 or any other value, the issue would be repeatedly triggered. So, actually it's a unconditional issue.

[ydspa]

you can't find a buffer that works at 90% percentile, it is even hard to work at 50%

percentile always can be found, such as medium number(50%).

On the opposite, mean value could be hard to find, those extreme high gas fee can make mean not converge.

Here is a mistake in thoughts, set buffer according the number of 90% percentile, not means the setting will work from 0% percentile to 90% percentile, actually it might only work for (85%, 95%).

[141345]

0.001, 0.02, 0.3, 4, 5, 6, 7, 8, 9, 10000.

For this skewed distribution, 90% percentile is 9.
Why only work for around 9?

[ydspa]

0.001, 0.02, 0.3, 4, 5, 6, 7, 8, 9, 10000.

For this skewed distribution, 90% percentile is 9. Why only work for around 9?
Let's say, at e9, keeper get $1, then at e8 it becomes $10, at e7 it would be $100...

[141345]

repeatedly triggered

My understanding, something like, it could be triggered 10-20% of the time in a certain time span, such as 1 month, 1 week.

It still falls into the category with condition:
Per criteria

The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future.

[ydspa]

My understanding about the probability of issue occurrence, which i learned when i work for other audit firms, are like this:

If the issue will occur even once with high probability in a reasonable period such as 3 months, then it's high probability.

It's far away from the thought that only if a issue will keep occurring in most the time, let's say, in 2 of the 3 months, then it's high probability.

So, in my opinion, if a issue might occur hundred times a month, it's obviously a high probability. And with high probability and high impact issue, we mark it as high.

Actually, we can think the situation much more simple:

If an exchange's service would be suspended randomly sum up to average 7 days per month, is it critical, high or medium?

I will choose critical though Sherlock doesn't have this level.

[hrishibhat]

Result:
High
Unique
After considering further points raised by Watson given the frequency of the condition mentioned in the example above. The impact is not just about the loss for the keeper but also keepers not submitting txs and executing orders. This can be looked at as a severe issue overall. Considering this issue a valid high.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: accepted

[jacksanford1]

From WatchPug:

https://github.com/equilibria-xyz/root/blob/ea30036560e54a9eb951a77c7a104e5cfe3c4d72/contracts/attribute/Kept/Kept.sol#L44-L62

https://github.com/equilibria-xyz/perennial-v2/blob/602218ca8cb62db649bf4b6a6722824e9ab20166/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L129-L133

The dynamicCalldata passed to the keep modifier is not the entire calldata. This will result in a wrong result of _calculateDynamicFee().

Recommendation
Consider calculating the length of the calldata before calling keep and change bytes memory dynamicCalldata to uint256 calldataLength.

[arjun-io]

From WatchPug:

https://github.com/equilibria-xyz/root/blob/ea30036560e54a9eb951a77c7a104e5cfe3c4d72/contracts/attribute/Kept/Kept.sol#L44-L62

https://github.com/equilibria-xyz/perennial-v2/blob/602218ca8cb62db649bf4b6a6722824e9ab20166/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L129-L133 The dynamicCalldata passed to the keep modifier is not the entire calldata. This will result in a wrong result of _calculateDynamicFee().

Recommendation Consider calculating the length of the calldata before calling keep and change bytes memory dynamicCalldata to uint256 calldataLength.

Passing in the whole msg.data is vulnerable to data expansion attacks, we chose to instead let the user of the library choose which portion of the data to incentivize. This is similar to how only the function with the incentivize modifier is counted with respect to the gas usage instead of the entire transaction.

The difference in real cost to the keeper can be accounted for via the premium and buffer parameters.