Skip to content

Commit b34c360

Browse files
authored
fix: fee related overflows (#20362)
There were a couple of potential issues with fee related overflows. - The prover cost was stored in 63 bits but it is possible to overflow that in multiple cases: - The prover cost per mana could be up to 64 bits, so as long as aztec < ether it can overflow for any sufficiently large prover cost per mana defined by governance - The L1 costs could be very high which together with token price and the raw proving cost moves it beyond the limit - The congestion cost likewise get impacted by the L1 costs and prover costs, but also the congestion multiplier of. - Given sufficiently high excess mana, it is possible for the congestion multiplier computation to overflow. - Given sufficiently high excess mana, it is possible for the next header to overflow when trying to compress it. To encounter any of these, the values must be absurd, e.g., we are talking multiple thousands if not millions of gwei in gasprices on L1, thousands of x of congestion multiplier and large small aztec token prices. However, as they are fairly simple to mitigate we will be doing so. Mitigation for the congestion multiplier is especially important, since overflows in there could be impossible to leave again as they "lag" and only depends on the excess. --- Mitigations: - For the fee header we apply to the values that does not depend on the current values only, e.g., `excessMana`, `congestionCost` and `proverCost` - When computing the summed fee we make a bound at uint128 - When computing the congestion multiplier we allow at most 100x the denominator to keep it safe, but still insanely high. The issues were pointed by spearbit, but as diving in found issues around the max value `summedMinFee` being potentially larger then `uint128` which would make it impossible to provide a matching header, thereby causing infinite reverts. Similar for the `excessMana` as it only depends on historical values could end up always reverting if overflowing. Hitting any of those cases is completely unrealistic (fee would be e^100). But better safe than sorry on this, as it could happen and that case would be a real pain.
2 parents 866c568 + 93be4fe commit b34c360

File tree

3 files changed

+422
-8
lines changed

3 files changed

+422
-8
lines changed

l1-contracts/src/core/libraries/compressed-data/fees/FeeStructs.sol

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
pragma solidity >=0.8.27;
44

55
import {CompressedSlot} from "@aztec/shared/libraries/CompressedTimeMath.sol";
6+
import {Math} from "@oz/utils/math/Math.sol";
67
import {SafeCast} from "@oz/utils/math/SafeCast.sol";
78

89
// We are using a type instead of a struct as we don't want to throw away a full 8 bits
@@ -102,13 +103,14 @@ library FeeHeaderLib {
102103
function compress(FeeHeader memory _feeHeader) internal pure returns (CompressedFeeHeader) {
103104
uint256 value = 0;
104105
value |= uint256(_feeHeader.manaUsed.toUint32());
105-
value |= uint256(_feeHeader.excessMana.toUint48()) << 32;
106+
// Cap excessMana to uint48 max to prevent overflow during compression.
107+
value |= Math.min(_feeHeader.excessMana, MASK_48_BITS) << 32;
106108
value |= uint256(_feeHeader.ethPerFeeAsset.toUint48()) << 80;
107-
value |= uint256(_feeHeader.congestionCost.toUint64()) << 128;
108-
109-
uint256 proverCost = uint256(_feeHeader.proverCost.toUint64());
110-
require(proverCost == proverCost & MASK_63_BITS);
111-
value |= proverCost << 192;
109+
// Cap congestionCost to uint64 max to prevent overflow during compression.
110+
// The uncapped value is still used for fee validation; this only affects storage.
111+
value |= Math.min(_feeHeader.congestionCost, MASK_64_BITS) << 128;
112+
// Cap proverCost to uint63 max to prevent overflow during compression.
113+
value |= Math.min(_feeHeader.proverCost, MASK_63_BITS) << 192;
112114

113115
// Preheat
114116
value |= 1 << 255;

l1-contracts/src/core/libraries/rollup/FeeLib.sol

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,11 @@ library FeeLib {
309309

310310
function congestionMultiplier(uint256 _numerator) internal view returns (uint256) {
311311
FeeStore storage feeStore = getStorage();
312-
return fakeExponential(MINIMUM_CONGESTION_MULTIPLIER, _numerator, feeStore.config.getCongestionUpdateFraction());
312+
uint256 denominator = feeStore.config.getCongestionUpdateFraction();
313+
// Cap the exponent to prevent overflow in the Taylor series.
314+
// At e^100, the multiplier is ~2.69e43 * MINIMUM_CONGESTION_MULTIPLIER, more than enough
315+
uint256 cappedNumerator = Math.min(_numerator, denominator * 100);
316+
return fakeExponential(MINIMUM_CONGESTION_MULTIPLIER, cappedNumerator, denominator);
313317
}
314318

315319
function computeManaLimit(uint256 _manaTarget) internal pure returns (uint256) {
@@ -342,7 +346,10 @@ library FeeLib {
342346
}
343347

344348
function summedMinFee(ManaMinFeeComponents memory _components) internal pure returns (uint256) {
345-
return _components.sequencerCost + _components.proverCost + _components.congestionCost;
349+
// Cap at uint128 max to ensure the fee can always be represented in the proposal header's
350+
// feePerL2Gas field (uint128). Without this cap, extreme congestion or parameter combinations
351+
// could produce fees that no valid header can represent, causing a liveness failure.
352+
return Math.min(_components.sequencerCost + _components.proverCost + _components.congestionCost, type(uint128).max);
346353
}
347354

348355
function getStorage() internal pure returns (FeeStore storage storageStruct) {

0 commit comments

Comments
 (0)