From b82b51d10f6c9c22536bed2f40574b0ed0e33966 Mon Sep 17 00:00:00 2001 From: soloseng <102702451+soloseng@users.noreply.github.com> Date: Wed, 25 Sep 2024 06:28:02 -0400 Subject: [PATCH] L1-L2-checks (#11225) --- .../contracts-0.8/governance/Validators.sol | 9 +-- .../protocol/contracts/common/Accounts.sol | 2 +- .../governance/DoubleSigningSlasher.sol | 2 +- .../contracts/governance/DowntimeSlasher.sol | 6 +- .../contracts/governance/Election.sol | 35 +++++---- .../governance/test/MockElection.sol | 4 +- .../governance/test/MockValidators.sol | 31 ++++---- .../test-sol/unit/common/Accounts.t.sol | 6 +- .../governance/validators/Validators.t.sol | 77 +++++++------------ .../unit/governance/voting/ReleaseGold.t.sol | 7 +- 10 files changed, 80 insertions(+), 99 deletions(-) diff --git a/packages/protocol/contracts-0.8/governance/Validators.sol b/packages/protocol/contracts-0.8/governance/Validators.sol index 829d322db6e..7b9354197f8 100644 --- a/packages/protocol/contracts-0.8/governance/Validators.sol +++ b/packages/protocol/contracts-0.8/governance/Validators.sol @@ -255,7 +255,7 @@ contract Validators is bytes calldata blsPublicKey, bytes calldata blsPop ) external nonReentrant returns (bool) { - allowOnlyL1(); + allowOnlyL1(); // For L2, use registerValidatorNoBls address account = getAccounts().validatorSignerToAccount(msg.sender); _isRegistrationAllowed(account); require(!isValidator(account) && !isValidatorGroup(account), "Already registered"); @@ -611,6 +611,7 @@ contract Validators is * @param validatorAccount The validator to deaffiliate from their affiliated validator group. */ function forceDeaffiliateIfValidator(address validatorAccount) external nonReentrant onlySlasher { + allowOnlyL1(); if (isValidator(validatorAccount)) { Validator storage validator = validators[validatorAccount]; if (validator.affiliation != address(0)) { @@ -829,9 +830,8 @@ contract Validators is uint256 epochNumber, uint256 index ) external view returns (address) { - allowOnlyL1(); require(isValidator(account), "Not a validator"); - require(epochNumber <= getEpochNumber(), "Epoch cannot be larger than current"); + require(epochNumber <= _getEpochNumber(), "Epoch cannot be larger than current"); MembershipHistory storage history = validators[account].membershipHistory; require(index < history.tail.add(history.numEntries), "index out of bounds"); require(index >= history.tail && history.numEntries > 0, "index out of bounds"); @@ -950,7 +950,6 @@ contract Validators is * @param delay Number of blocks to delay the update */ function setCommissionUpdateDelay(uint256 delay) public onlyOwner { - allowOnlyL1(); require(delay != commissionUpdateDelay, "commission update delay not changed"); commissionUpdateDelay = delay; emit CommissionUpdateDelaySet(delay); @@ -962,7 +961,6 @@ contract Validators is * @return True upon success. */ function setMaxGroupSize(uint256 size) public onlyOwner returns (bool) { - allowOnlyL1(); require(0 < size, "Max group size cannot be zero"); require(size != maxGroupSize, "Max group size not changed"); maxGroupSize = size; @@ -976,7 +974,6 @@ contract Validators is * @return True upon success. */ function setMembershipHistoryLength(uint256 length) public onlyOwner returns (bool) { - allowOnlyL1(); require(0 < length, "Membership history length cannot be zero"); require(length != membershipHistoryLength, "Membership history length not changed"); membershipHistoryLength = length; diff --git a/packages/protocol/contracts/common/Accounts.sol b/packages/protocol/contracts/common/Accounts.sol index af5a6885dcf..13fed26f9c5 100644 --- a/packages/protocol/contracts/common/Accounts.sol +++ b/packages/protocol/contracts/common/Accounts.sol @@ -567,7 +567,7 @@ contract Accounts is * be greater than 1. * @dev Use `deletePaymentDelegation` to unset the payment delegation. */ - function setPaymentDelegation(address beneficiary, uint256 fraction) public onlyL1 { + function setPaymentDelegation(address beneficiary, uint256 fraction) public { require(isAccount(msg.sender), "Must first register address with Account.createAccount"); require(beneficiary != address(0), "Beneficiary cannot be address 0x0"); FixidityLib.Fraction memory f = FixidityLib.wrap(fraction); diff --git a/packages/protocol/contracts/governance/DoubleSigningSlasher.sol b/packages/protocol/contracts/governance/DoubleSigningSlasher.sol index b9aef3f737c..b36359cd07d 100644 --- a/packages/protocol/contracts/governance/DoubleSigningSlasher.sol +++ b/packages/protocol/contracts/governance/DoubleSigningSlasher.sol @@ -116,7 +116,7 @@ contract DoubleSigningSlasher is ICeloVersionedContract, SlasherUtil { uint256 index, bytes memory headerA, bytes memory headerB - ) public view returns (uint256) { + ) public view onlyL1 returns (uint256) { require(hashHeader(headerA) != hashHeader(headerB), "Block hashes have to be different"); uint256 blockNumber = getBlockNumberFromHeader(headerA); require( diff --git a/packages/protocol/contracts/governance/DowntimeSlasher.sol b/packages/protocol/contracts/governance/DowntimeSlasher.sol index d9f4f68a0ab..51a099cc542 100644 --- a/packages/protocol/contracts/governance/DowntimeSlasher.sol +++ b/packages/protocol/contracts/governance/DowntimeSlasher.sol @@ -170,7 +170,7 @@ contract DowntimeSlasher is ICeloVersionedContract, SlasherUtil { function getBitmapForInterval( uint256 startBlock, uint256 endBlock - ) public view returns (bytes32) { + ) public view onlyL1 returns (bytes32) { require(endBlock >= startBlock, "endBlock must be greater or equal than startBlock"); // The signature bitmap for block N is stored in block N+1. // The latest block is `block.number - 1`, which stores the signature bitmap for @@ -215,7 +215,7 @@ contract DowntimeSlasher is ICeloVersionedContract, SlasherUtil { uint256 startBlock, uint256 endBlock, uint256 signerIndex - ) public view returns (bool) { + ) public view onlyL1 returns (bool) { require(signerIndex < numberValidatorsInSet(startBlock), "bad validator index at start block"); require( isBitmapSetForInterval(startBlock, endBlock), @@ -250,7 +250,7 @@ contract DowntimeSlasher is ICeloVersionedContract, SlasherUtil { uint256[] memory startBlocks, uint256[] memory endBlocks, uint256[] memory signerIndices - ) public view returns (bool) { + ) public view onlyL1 returns (bool) { require(startBlocks.length > 0, "requires at least one interval"); require( startBlocks.length == endBlocks.length, diff --git a/packages/protocol/contracts/governance/Election.sol b/packages/protocol/contracts/governance/Election.sol index 653348dbeb3..5e1c15c4684 100644 --- a/packages/protocol/contracts/governance/Election.sol +++ b/packages/protocol/contracts/governance/Election.sol @@ -637,10 +637,7 @@ contract Election is */ function hasActivatablePendingVotes(address account, address group) external view returns (bool) { PendingVote storage pendingVote = votes.pending.forGroup[group].byAccount[account]; - if (isL2()) { - return pendingVote.epoch < getEpochManager().getCurrentEpochNumber() && pendingVote.value > 0; - } - return pendingVote.epoch < getEpochNumber() && pendingVote.value > 0; + return pendingVote.epoch < _getEpochNumber() && pendingVote.value > 0; } /** @@ -1010,14 +1007,9 @@ contract Election is function _activate(address group, address account) internal onlyWhenNotBlocked returns (bool) { PendingVote storage pendingVote = votes.pending.forGroup[group].byAccount[account]; - if (isL2()) { - require( - pendingVote.epoch < getEpochManager().getCurrentEpochNumber(), - "Pending vote epoch not passed" - ); - } else { - require(pendingVote.epoch < getEpochNumber(), "Pending vote epoch not passed"); - } + + require(pendingVote.epoch < _getEpochNumber(), "Pending vote epoch not passed"); + uint256 value = pendingVote.value; require(value > 0, "Vote value cannot be zero"); decrementPendingVotes(group, account, value); @@ -1165,11 +1157,7 @@ contract Election is PendingVote storage pendingVote = groupPending.byAccount[account]; pendingVote.value = pendingVote.value.add(value); - if (isL2()) { - pendingVote.epoch = getEpochManager().getCurrentEpochNumber(); - } else { - pendingVote.epoch = getEpochNumber(); - } + pendingVote.epoch = _getEpochNumber(); } /** @@ -1290,4 +1278,17 @@ contract Election is value.mul(votes.active.forGroup[group].total).div(votes.active.forGroup[group].totalUnits); } } + + /** + * @notice Returns the epoch number. + * @return Current epoch number. + */ + function _getEpochNumber() private view returns (uint256) { + // TODO remove this after L2 is fully implemented + if (isL2()) { + return getEpochManager().getCurrentEpochNumber(); + } else { + return getEpochNumber(); + } + } } diff --git a/packages/protocol/contracts/governance/test/MockElection.sol b/packages/protocol/contracts/governance/test/MockElection.sol index 4dd6961b584..b2b6034383a 100644 --- a/packages/protocol/contracts/governance/test/MockElection.sol +++ b/packages/protocol/contracts/governance/test/MockElection.sol @@ -17,7 +17,7 @@ contract MockElection is IsL2Check { isIneligible[account] = true; } - function markGroupEligible(address account, address, address) external onlyL1 { + function markGroupEligible(address account, address, address) external { isEligible[account] = true; } @@ -33,7 +33,7 @@ contract MockElection is IsL2Check { electedValidators = _electedValidators; } - function vote(address, uint256, address, address) external onlyL1 returns (bool) { + function vote(address, uint256, address, address) external returns (bool) { return true; } diff --git a/packages/protocol/contracts/governance/test/MockValidators.sol b/packages/protocol/contracts/governance/test/MockValidators.sol index 54a87bdd5a3..d47f44b447c 100644 --- a/packages/protocol/contracts/governance/test/MockValidators.sol +++ b/packages/protocol/contracts/governance/test/MockValidators.sol @@ -27,7 +27,6 @@ contract MockValidators is IValidators, IsL2Check { uint256 private numRegisteredValidators; function updateEcdsaPublicKey(address, address, bytes calldata) external returns (bool) { - allowOnlyL1(); return true; } @@ -50,12 +49,7 @@ contract MockValidators is IValidators, IsL2Check { isValidatorGroup[group] = true; } - function getValidatorsGroup(address validator) external view returns (address) { - return affiliations[validator]; - } - function affiliate(address group) external returns (bool) { - allowOnlyL1(); affiliations[msg.sender] = group; return true; } @@ -84,20 +78,15 @@ contract MockValidators is IValidators, IsL2Check { } function halveSlashingMultiplier(address) external { - allowOnlyL1(); // TODO remove + allowOnlyL1(); } function forceDeaffiliateIfValidator(address validator) external { - allowOnlyL1(); // TODO remove + allowOnlyL1(); } - function getTopGroupValidators(address group, uint256 n) public view returns (address[] memory) { - require(n <= members[group].length); - address[] memory validators = new address[](n); - for (uint256 i = 0; i < n; i = i.add(1)) { - validators[i] = members[group][i]; - } - return validators; + function getValidatorsGroup(address validator) external view returns (address) { + return affiliations[validator]; } function getTopGroupValidatorsAccounts( @@ -148,7 +137,6 @@ contract MockValidators is IValidators, IsL2Check { } function groupMembershipInEpoch(address addr, uint256, uint256) external view returns (address) { - allowOnlyL1(); return affiliations[addr]; } @@ -156,6 +144,15 @@ contract MockValidators is IValidators, IsL2Check { return members[group].length; } + function getTopGroupValidators(address group, uint256 n) public view returns (address[] memory) { + require(n <= members[group].length); + address[] memory validators = new address[](n); + for (uint256 i = 0; i < n; i = i.add(1)) { + validators[i] = members[group][i]; + } + return validators; + } + // Not implemented in mock, added here to support the interface // without the interface, missing function erros get hard to debug @@ -286,7 +283,7 @@ contract MockValidators is IValidators, IsL2Check { revert("Method not implemented in mock"); } - function distributeEpochPaymentsFromSigner(address, uint256) external returns (uint256) { + function distributeEpochPaymentsFromSigner(address, uint256) external onlyL1 returns (uint256) { revert("Method not implemented in mock"); } diff --git a/packages/protocol/test-sol/unit/common/Accounts.t.sol b/packages/protocol/test-sol/unit/common/Accounts.t.sol index 9fe254a2365..94ee49dac5b 100644 --- a/packages/protocol/test-sol/unit/common/Accounts.t.sol +++ b/packages/protocol/test-sol/unit/common/Accounts.t.sol @@ -608,11 +608,13 @@ contract AccountsTest_setPaymentDelegation is AccountsTest { assertEq(realFraction, fraction); } - function test_Revert_SetPaymentDelegation_WhenL2() public { + function test_ShouldSetAnAddressAndAFraction_WhenL2() public { _whenL2(); accounts.createAccount(); - vm.expectRevert("This method is no longer supported in L2."); accounts.setPaymentDelegation(beneficiary, fraction); + (address realBeneficiary, uint256 realFraction) = accounts.getPaymentDelegation(address(this)); + assertEq(realBeneficiary, beneficiary); + assertEq(realFraction, fraction); } function test_ShouldNotAllowFractionGreaterThan1() public { diff --git a/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol b/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol index 15dd3b2ddc2..6af19e999b8 100644 --- a/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol +++ b/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol @@ -506,10 +506,12 @@ contract ValidatorsTest_Initialize is ValidatorsTest { assertEq(actual, commissionUpdateDelay, "Wrong commissionUpdateDelay."); } - function test_Reverts_setCommissionUpdateDelay_WhenL2() public { + function test_ShouldsetCommissionUpdateDelay_WhenL2() public { _whenL2(); - vm.expectRevert("This method is no longer supported in L2."); - validators.setCommissionUpdateDelay(commissionUpdateDelay); + validators.setCommissionUpdateDelay(5); + + uint256 actual = validators.getCommissionUpdateDelay(); + assertEq(actual, 5, "Wrong commissionUpdateDelay."); } function test_shouldHaveSetDowntimeGracePeriod() public { @@ -537,13 +539,14 @@ contract ValidatorsTest_SetMembershipHistoryLength is ValidatorsTest { assertEq(validators.getMembershipHistoryLength(), newLength); } - function test_Reverts_SetTheMembershipHistoryLength_WhenL2() public { - _whenL2(); - vm.expectRevert("This method is no longer supported in L2."); + function test_Emits_MembershipHistoryLengthSet() public { + vm.expectEmit(true, true, true, true); + emit MembershipHistoryLengthSet(newLength); validators.setMembershipHistoryLength(newLength); } - function test_Emits_MembershipHistoryLengthSet() public { + function test_Emits_MembershipHistoryLengthSet_WhenL2() public { + _whenL2(); vm.expectEmit(true, true, true, true); emit MembershipHistoryLengthSet(newLength); validators.setMembershipHistoryLength(newLength); @@ -561,9 +564,10 @@ contract ValidatorsTest_SetMaxGroupSize is ValidatorsTest { event MaxGroupSizeSet(uint256 size); - function test_Reverts_SetMaxGroupSize_WhenL2() public { + function test_Emits_MaxGroupSizeSet_WhenL2() public { _whenL2(); - vm.expectRevert("This method is no longer supported in L2."); + vm.expectEmit(true, true, true, true); + emit MaxGroupSizeSet(newSize); validators.setMaxGroupSize(newSize); } @@ -2142,14 +2146,16 @@ contract ValidatorsTest_AddMember is ValidatorsTest { assertEq(members, expectedMembersList); } - function test_Reverts_AddFirstMemberToTheList_WhenL2() public { - _whenL2(); + function test_ShouldAddMemberToTheList_WhenL2() public { address[] memory expectedMembersList = new address[](1); expectedMembersList[0] = validator; - + _whenL2(); vm.prank(group); - vm.expectRevert("This method is no longer supported in L2."); validators.addFirstMember(validator, address(0), address(0)); + + (address[] memory members, , , , , , ) = validators.getValidatorGroup(group); + + assertEq(members, expectedMembersList); } function test_ShouldUpdateGroupSizeHistory() public { @@ -3635,35 +3641,9 @@ contract ValidatorsTest_ForceDeaffiliateIfValidator is ValidatorsTest { function test_ShouldSendValidatorPayment_WhenL2() public { _whenL2(); - vm.expectEmit(true, true, true, true); - emit SendValidatorPaymentCalled(validator); - vm.prank(paymentDelegatee); - validators.forceDeaffiliateIfValidator(validator); - } -} -contract ValidatorsTest_ForceDeaffiliateIfValidator_L2 is ValidatorsTest { - function setUp() public { - super.setUp(); - - _registerValidatorHelper(validator, validatorPk); - _registerValidatorGroupHelper(group, 1); - - vm.prank(validator); - validators.affiliate(group); - _whenL2(); - lockedGold.addSlasherTest(paymentDelegatee); - } - - function test_ShouldSucceed_WhenSenderIsWhitelistedSlashingAddress() public { + vm.expectRevert("This method is no longer supported in L2."); vm.prank(paymentDelegatee); validators.forceDeaffiliateIfValidator(validator); - (, , address affiliation, , ) = validators.getValidator(validator); - assertEq(affiliation, address(0)); - } - - function test_Reverts_WhenSenderNotApprovedAddress() public { - vm.expectRevert("Only registered slasher can call"); - validators.forceDeaffiliateIfValidator(validator); } } @@ -3740,21 +3720,22 @@ contract ValidatorsTest_GroupMembershipInEpoch is ValidatorsTest { } } } - - function test_Reverts_GroupMembershipInEpoch_WhenL2() public { + function test_ShouldCorrectlyGetGroupAddressForExactEpochNumbers_WhenL2() public { _whenL2(); for (uint256 i = 0; i < epochInfoList.length; i++) { address _group = epochInfoList[i].groupy; if (epochInfoList.length.sub(i) <= membershipHistoryLength) { - vm.expectRevert("This method is no longer supported in L2."); - validators.groupMembershipInEpoch( - validator, - epochInfoList[i].epochNumber, - uint256(1).add(i) + assertEq( + validators.groupMembershipInEpoch( + validator, + epochInfoList[i].epochNumber, + uint256(1).add(i) + ), + _group ); } else { - vm.expectRevert("This method is no longer supported in L2."); + vm.expectRevert("index out of bounds"); validators.groupMembershipInEpoch( validator, epochInfoList[i].epochNumber, diff --git a/packages/protocol/test-sol/unit/governance/voting/ReleaseGold.t.sol b/packages/protocol/test-sol/unit/governance/voting/ReleaseGold.t.sol index 23dc0ea9977..f5aae08777c 100644 --- a/packages/protocol/test-sol/unit/governance/voting/ReleaseGold.t.sol +++ b/packages/protocol/test-sol/unit/governance/voting/ReleaseGold.t.sol @@ -1115,9 +1115,8 @@ contract ReleaseGoldTest_AuthorizeWithPublicKeys is ReleaseGoldTest { assertEq(accounts.validatorSignerToAccount(authorized), address(releaseGold)); } - function test_Reverts_WhenAuthorizeValidatorSignerWithPublicKeyCalledOnL2() public { + function test_ShouldSetTheAuthorizedKeys_WhenUsingECDSAPublickKey_WhenL2() public { _whenL2(); - vm.expectRevert("This method is no longer supported in L2."); vm.prank(beneficiary); releaseGold.authorizeValidatorSignerWithPublicKey( address(uint160(authorized)), @@ -1126,6 +1125,10 @@ contract ReleaseGoldTest_AuthorizeWithPublicKeys is ReleaseGoldTest { s, ecdsaPublicKey ); + + assertEq(accounts.authorizedBy(authorized), address(releaseGold)); + assertEq(accounts.getValidatorSigner(address(releaseGold)), authorized); + assertEq(accounts.validatorSignerToAccount(authorized), address(releaseGold)); } function test_Reverts_WhenAuthorizeValidatorSignerWithKeysCalledOnL2() public {