Open sherlock-admin opened 1 year ago
1 comment(s) were left on this issue during the judging contest.
141345 commented:
m
The Kept
helper supports a buffer amount which can be used for this use case.
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
Escalate
Using
Kept
buffer can't solve this problem, as the ratio ofL1GasPrice / L2GasPrice
changes in every block and can vary with a extreme wide range. Let's sayCurrentL1GasPrice = 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 thanL1RollupFee
, keepers suffer lossesL1RollupFee = 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 thanL1RollupFee
, 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
andblock.basefee
could makekeeperReward > settlementFee
, then a new attack vector become feasible,keepers can set1 wei
orders by self to drain fund from protocol, asself.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.
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!
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.
Fixed Update Kept for L2s: https://github.com/equilibria-xyz/root/pull/74 and https://github.com/equilibria-xyz/root/pull/76 Update Pyth Oracle for L2: https://github.com/equilibria-xyz/perennial-v2/pull/85
@ydspa tend to agree with this comment: https://github.com/sherlock-audit/2023-07-perennial-judging/issues/91#issuecomment-1697458799 A valid medium
@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:
buffer
, it can only work in a extreme narrow Ratio
range as compared to (e1, e9)
.100%
sure this bug would be repeatedly triggered during the long lifetime of the protocol.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.
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.
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
.
Even the distribution is Pareto distribution (power law), percentile still works. 50%, 90% can be found.
Even the distribution is Pareto distribution (power law), percentile still works. 50%, 90% can be found.
Give your specific parameters please
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.
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
The main point is, there is some parameter, considering the trade off, can balance the loss and overpayment. e8 is just an example.
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.
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%)
.
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?
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 ate8
it becomes $10, ate7
it would be $100...
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.
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.
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.
From WatchPug:
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.
From WatchPug:
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.
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.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.
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