Skip to content

Commit 3a13674

Browse files
authored
fix: misc minor contract fixes (#20423)
Fixing misc minor issues: - The EIP-712 comment in `TallySlashingProposer` had wrong order of arguments - The `TallySlashingProposer` had a explicit MUST be >0 in the comments but did not enforce it in the code - Comment said 64 bits for a 32 bit value in `FeeConfig` - Unused `feeHeaders` in the `FeeLib`, was not removed when we refactored the storage long ago - Require the bond of the escape hatch to be > 0 - Remove the amount from the `CandidateJoined` since it was just a immutable value - Remove the unsed `proposerIndex` and `slot` from the `verifyAttestations` - Ensure that we do not get stale values from `getVotes` on the tally slashing proposer - The `getRound` on the tally slashing proposer had a few unnecessary check that the `_getRoundData` was already doing.
2 parents e4712cd + 5f4e35d commit 3a13674

File tree

13 files changed

+225
-54
lines changed

13 files changed

+225
-54
lines changed

l1-contracts/src/core/EscapeHatch.sol

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,9 @@ contract EscapeHatch is IEscapeHatch {
111111

112112
require(_frequency > _activeDuration, Errors.EscapeHatch__InvalidConfiguration());
113113

114+
// BOND_SIZE must be non-zero to ensure "something at stake"
115+
require(_bondSize > 0, Errors.EscapeHatch__InvalidConfiguration());
116+
114117
// FAILED_HATCH_PUNISHMENT must be <= BOND_SIZE to avoid underflow
115118
require(_failedHatchPunishment <= _bondSize, Errors.EscapeHatch__InvalidConfiguration());
116119

@@ -157,7 +160,7 @@ contract EscapeHatch is IEscapeHatch {
157160

158161
BOND_TOKEN.safeTransferFrom(candidate, address(this), BOND_SIZE);
159162

160-
emit CandidateJoined(candidate, BOND_SIZE);
163+
emit CandidateJoined(candidate);
161164
}
162165

163166
/**

l1-contracts/src/core/interfaces/IEscapeHatch.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ struct CandidateInfo {
3636
}
3737

3838
interface IEscapeHatchCore {
39-
event CandidateJoined(address indexed candidate, uint256 amount);
39+
event CandidateJoined(address indexed candidate);
4040
event CandidateExitInitiated(address indexed candidate, uint256 exitableAt);
4141
event CandidateExited(address indexed candidate, uint256 amountReturned);
4242
event CandidateSelected(Hatch indexed hatch, address indexed candidate);

l1-contracts/src/core/libraries/Errors.sol

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ library Errors {
227227
error TallySlashingProposer__InvalidEpochIndex(uint256 epochIndex, uint256 roundSizeInEpochs);
228228
error TallySlashingProposer__VoteSizeTooBig(uint256 voteSize, uint256 maxSize);
229229
error TallySlashingProposer__VotesMustBeMultipleOf4(uint256 votes);
230+
error TallySlashingProposer__SlashAmountMustBeGtZero(string info);
230231

231232
// SlashPayloadLib
232233
error SlashPayload_ArraySizeMismatch(uint256 expected, uint256 actual);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ function subEthValue(EthValue _a, EthValue _b) pure returns (EthValue) {
3838

3939
using {addEthValue as +, subEthValue as -} for EthValue global;
4040

41-
// 64 bit manaTarget, 128 bit congestionUpdateFraction, 64 bit provingCostPerMana
41+
// 32 bit manaTarget, 128 bit congestionUpdateFraction, 64 bit provingCostPerMana
4242
type CompressedFeeConfig is uint256;
4343

4444
struct FeeConfig {

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -303,8 +303,7 @@ library EpochProofLib {
303303
bytes32 providedAttestationsHash = keccak256(abi.encode(_attestations));
304304
require(providedAttestationsHash == checkpointLog.attestationsHash, Errors.Rollup__InvalidAttestations());
305305

306-
// Get the slot and epoch for the last checkpoint
307-
Slot slot = checkpointLog.slotNumber.decompress();
306+
// Get the epoch for the last checkpoint
308307
Epoch epoch = STFLib.getEpochForCheckpoint(_endCheckpointNumber);
309308

310309
// Check if this is an escape hatch epoch - skip attestation verification if so
@@ -322,7 +321,7 @@ library EpochProofLib {
322321
}
323322
}
324323

325-
ValidatorSelectionLib.verifyAttestations(slot, epoch, _attestations, checkpointLog.payloadDigest);
324+
ValidatorSelectionLib.verifyAttestations(epoch, _attestations, checkpointLog.payloadDigest);
326325
}
327326

328327
/**

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ struct ManaMinFeeComponents {
8585
struct FeeStore {
8686
CompressedFeeConfig config;
8787
L1GasOracleValues l1GasOracleValues;
88-
mapping(uint256 checkpointNumber => CompressedFeeHeader feeHeader) feeHeaders;
8988
}
9089

9190
library FeeLib {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ library RollupOperationsExtLib {
5555

5656
Slot slot = _args.header.slotNumber;
5757
Epoch epoch = slot.epochFromSlot();
58-
ValidatorSelectionLib.verifyAttestations(slot, epoch, _attestations, _args.digest);
58+
ValidatorSelectionLib.verifyAttestations(epoch, _attestations, _args.digest);
5959
ValidatorSelectionLib.verifyProposer(
6060
slot, epoch, _attestations, _signers, _args.digest, _attestationsAndSignersSignature, false
6161
);

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,12 @@ library ValidatorSelectionLib {
110110
/**
111111
* @dev Stack struct used in verifyAttestations to avoid stack too deep errors
112112
* Used when reconstructing the committee commitment from the attestations
113-
* @param proposerIndex Index of the proposer within the committee
114113
* @param index Working index for iteration (unused in current implementation)
115114
* @param needed Number of signatures required (2/3 + 1 of committee size)
116115
* @param signaturesRecovered Number of valid signatures found
117116
* @param reconstructedCommittee Array of committee member addresses reconstructed from attestations
118117
*/
119118
struct VerifyStack {
120-
uint256 proposerIndex;
121119
uint256 index;
122120
uint256 needed;
123121
uint256 signaturesRecovered;
@@ -308,7 +306,6 @@ library ValidatorSelectionLib {
308306
* directly from calldata.
309307
*
310308
* Skips validation entirely if target committee size is 0 (test configurations).
311-
* @param _slot The slot of the checkpoint
312309
* @param _epochNumber The epoch of the checkpoint
313310
* @param _attestations The packed signatures and addresses of committee members
314311
* @param _digest The digest of the checkpoint that attestations are signed over
@@ -317,12 +314,9 @@ library ValidatorSelectionLib {
317314
* stored commitment
318315
* @custom:reverts Errors.ValidatorSelection__EpochNotStable if the requested epoch is not stable
319316
*/
320-
function verifyAttestations(
321-
Slot _slot,
322-
Epoch _epochNumber,
323-
CommitteeAttestations memory _attestations,
324-
bytes32 _digest
325-
) internal {
317+
function verifyAttestations(Epoch _epochNumber, CommitteeAttestations memory _attestations, bytes32 _digest)
318+
internal
319+
{
326320
(bytes32 committeeCommitment, uint256 targetCommitteeSize) = getCommitteeCommitmentAt(_epochNumber);
327321

328322
// If the rollup is *deployed* with a target committee size of 0, we skip the validation.
@@ -333,7 +327,6 @@ library ValidatorSelectionLib {
333327
}
334328

335329
VerifyStack memory stack = VerifyStack({
336-
proposerIndex: computeProposerIndex(_epochNumber, _slot, getSampleSeed(_epochNumber), targetCommitteeSize),
337330
needed: (targetCommitteeSize << 1) / 3 + 1, // targetCommitteeSize * 2 / 3 + 1, but cheaper
338331
index: 0,
339332
signaturesRecovered: 0,

l1-contracts/src/core/slashing/TallySlashingProposer.sol

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ contract TallySlashingProposer is EIP712 {
151151

152152
/**
153153
* @notice EIP-712 type hash for the Vote struct used in signature verification
154-
* @dev Defines the structure: Vote(uint256 slot,bytes votes) for EIP-712 signing
154+
* @dev Defines the structure: Vote(bytes votes,uint256 slot) for EIP-712 signing
155155
*/
156156
bytes32 public constant VOTE_TYPEHASH = keccak256("Vote(bytes votes,uint256 slot)");
157157

@@ -361,6 +361,14 @@ contract TallySlashingProposer is EIP712 {
361361
COMMITTEE_SIZE * ROUND_SIZE_IN_EPOCHS % 4 == 0,
362362
Errors.TallySlashingProposer__VotesMustBeMultipleOf4(COMMITTEE_SIZE * ROUND_SIZE_IN_EPOCHS)
363363
);
364+
365+
// Defense in depth: the ordering constraints (small <= medium <= large) above mean that
366+
// if medium or large is zero, small must also be zero, so the small check would fire first.
367+
// We keep all three checks explicitly for clarity and to guard against future refactors
368+
// that might remove or reorder the sorting constraints.
369+
require(SLASH_AMOUNT_SMALL > 0, Errors.TallySlashingProposer__SlashAmountMustBeGtZero("small"));
370+
require(SLASH_AMOUNT_MEDIUM > 0, Errors.TallySlashingProposer__SlashAmountMustBeGtZero("medium"));
371+
require(SLASH_AMOUNT_LARGE > 0, Errors.TallySlashingProposer__SlashAmountMustBeGtZero("large"));
364372
}
365373

366374
/**
@@ -562,16 +570,8 @@ contract TallySlashingProposer is EIP712 {
562570
* @return voteCount The total number of votes that have been cast in this round by proposers
563571
*/
564572
function getRound(SlashRound _round) external view returns (bool isExecuted, uint256 voteCount) {
565-
SlashRound currentRound = getCurrentRound();
566-
567573
// Load round data from the circular storage
568-
RoundData memory roundData = _getRoundData(_round, currentRound);
569-
570-
// If we have not written to this round yet, return fresh round data
571-
if (roundData.roundNumber != _round) {
572-
return (false, 0);
573-
}
574-
574+
RoundData memory roundData = _getRoundData(_round, getCurrentRound());
575575
return (roundData.executed, roundData.voteCount);
576576
}
577577

@@ -593,6 +593,17 @@ contract TallySlashingProposer is EIP712 {
593593
*/
594594
function getVotes(SlashRound _round, uint256 _index) external view returns (bytes memory) {
595595
uint256 expectedLength = COMMITTEE_SIZE * ROUND_SIZE_IN_EPOCHS / 4;
596+
597+
// _getRoundVotes reverts if _round is out of the roundabout range.
598+
// But within-range the storage slot may still hold stale data from a
599+
// previous round that mapped to the same circular index. Guard against
600+
// that by checking the authoritative round metadata first.
601+
SlashRound currentRound = getCurrentRound();
602+
RoundData memory roundData = _getRoundData(_round, currentRound);
603+
if (roundData.voteCount == 0) {
604+
return new bytes(expectedLength);
605+
}
606+
596607
bytes32[4] storage voteSlots = _getRoundVotes(_round).votes[_index];
597608
return _loadVoteDataFromStorage(voteSlots, expectedLength);
598609
}

l1-contracts/test/escape-hatch/unit/constructor.t.sol

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,34 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
127127
_;
128128
}
129129

130+
function test_WhenBOND_SIZEEQ0(uint256 _lagInHatches, uint256 _activeDuration, uint256 _frequency)
131+
external
132+
whenLAG_IN_HATCHESGE1(_lagInHatches)
133+
whenACTIVE_DURATIONGE2(_activeDuration)
134+
whenFREQUENCYGTLAG_IN_EPOCHS_FOR_SET_SIZE(_frequency)
135+
whenFREQUENCYGTACTIVE_DURATION(_frequency)
136+
{
137+
// it should revert {EscapeHatch__InvalidConfiguration}
138+
vm.expectRevert(Errors.EscapeHatch__InvalidConfiguration.selector);
139+
new EscapeHatch(
140+
address(rollup),
141+
address(bondToken),
142+
0,
143+
0,
144+
0,
145+
fuzzFrequency,
146+
fuzzActiveDuration,
147+
fuzzLagInHatches,
148+
DEFAULT_PROPOSING_EXIT_DELAY
149+
);
150+
}
151+
152+
modifier whenBOND_SIZEGT0(uint96 _bondSize) {
153+
// Leave room for punishment/tax > bondSize tests
154+
fuzzBondSize = uint96(bound(_bondSize, 1, type(uint96).max - 1));
155+
_;
156+
}
157+
130158
function test_WhenFAILED_HATCH_PUNISHMENTGTBOND_SIZE(
131159
uint256 _lagInHatches,
132160
uint256 _activeDuration,
@@ -139,16 +167,16 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
139167
whenACTIVE_DURATIONGE2(_activeDuration)
140168
whenFREQUENCYGTLAG_IN_EPOCHS_FOR_SET_SIZE(_frequency)
141169
whenFREQUENCYGTACTIVE_DURATION(_frequency)
170+
whenBOND_SIZEGT0(_bondSize)
142171
{
143172
// it should revert {EscapeHatch__InvalidConfiguration}
144-
_bondSize = uint96(bound(_bondSize, 1, type(uint96).max - 1));
145-
_punishment = uint96(bound(_punishment, uint256(_bondSize) + 1, type(uint96).max));
173+
_punishment = uint96(bound(_punishment, uint256(fuzzBondSize) + 1, type(uint96).max));
146174

147175
vm.expectRevert(Errors.EscapeHatch__InvalidConfiguration.selector);
148176
new EscapeHatch(
149177
address(rollup),
150178
address(bondToken),
151-
_bondSize,
179+
fuzzBondSize,
152180
0,
153181
_punishment,
154182
fuzzFrequency,
@@ -158,9 +186,7 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
158186
);
159187
}
160188

161-
modifier whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(uint96 _bondSize, uint96 _punishment) {
162-
// Leave room for tax > bondSize test
163-
fuzzBondSize = uint96(bound(_bondSize, 1, type(uint96).max - 1));
189+
modifier whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(uint96 _punishment) {
164190
fuzzFailedHatchPunishment = uint96(bound(_punishment, 0, fuzzBondSize));
165191
_;
166192
}
@@ -178,7 +204,8 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
178204
whenACTIVE_DURATIONGE2(_activeDuration)
179205
whenFREQUENCYGTLAG_IN_EPOCHS_FOR_SET_SIZE(_frequency)
180206
whenFREQUENCYGTACTIVE_DURATION(_frequency)
181-
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_bondSize, _punishment)
207+
whenBOND_SIZEGT0(_bondSize)
208+
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_punishment)
182209
{
183210
// it should revert {EscapeHatch__InvalidConfiguration}
184211
_tax = uint96(bound(_tax, uint256(fuzzBondSize) + 1, type(uint96).max));
@@ -216,7 +243,8 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
216243
whenACTIVE_DURATIONGE2(_activeDuration)
217244
whenFREQUENCYGTLAG_IN_EPOCHS_FOR_SET_SIZE(_frequency)
218245
whenFREQUENCYGTACTIVE_DURATION(_frequency)
219-
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_bondSize, _punishment)
246+
whenBOND_SIZEGT0(_bondSize)
247+
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_punishment)
220248
whenWITHDRAWAL_TAXLEBOND_SIZE(_tax)
221249
{
222250
// it should revert {EscapeHatch__InvalidConfiguration}
@@ -250,7 +278,8 @@ contract EscapeHatchConstructorTest is EscapeHatchBase {
250278
whenACTIVE_DURATIONGE2(_activeDuration)
251279
whenFREQUENCYGTLAG_IN_EPOCHS_FOR_SET_SIZE(_frequency)
252280
whenFREQUENCYGTACTIVE_DURATION(_frequency)
253-
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_bondSize, _punishment)
281+
whenBOND_SIZEGT0(_bondSize)
282+
whenFAILED_HATCH_PUNISHMENTLEBOND_SIZE(_punishment)
254283
whenWITHDRAWAL_TAXLEBOND_SIZE(_tax)
255284
{
256285
// it should set ROLLUP immutable

0 commit comments

Comments
 (0)