diff --git a/contracts/src/LightClient.sol b/contracts/src/LightClient.sol index f3110da1f..4778465ec 100644 --- a/contracts/src/LightClient.sol +++ b/contracts/src/LightClient.sol @@ -174,13 +174,14 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { function getVersion() public pure + virtual returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion) { return (1, 0, 0); } /// @notice only the owner can authorize an upgrade - function _authorizeUpgrade(address newImplementation) internal override onlyOwner { + function _authorizeUpgrade(address newImplementation) internal virtual override onlyOwner { emit Upgrade(newImplementation); } @@ -237,7 +238,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { function newFinalizedState( LightClientState memory newState, IPlonkVerifier.PlonkProof memory proof - ) external { + ) external virtual { //revert if we're in permissionedProver mode and the permissioned prover has not been set if (permissionedProverEnabled && msg.sender != permissionedProver) { if (permissionedProver == address(0)) { @@ -287,12 +288,12 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { } /// @dev Simple getter function for the genesis state - function getGenesisState() public view returns (LightClientState memory) { + function getGenesisState() public view virtual returns (LightClientState memory) { return states[genesisState]; } /// @dev Simple getter function for the finalized state - function getFinalizedState() public view returns (LightClientState memory) { + function getFinalizedState() public view virtual returns (LightClientState memory) { return states[finalizedState]; } @@ -322,7 +323,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { /// @notice Advance to the next epoch (without any precondition check!) /// @dev This meant to be invoked only internally after appropriate precondition checks are done - function _advanceEpoch() private { + function _advanceEpoch() internal virtual { bytes32 newStakeTableComm = computeStakeTableComm(states[finalizedState]); votingStakeTableCommitment = frozenStakeTableCommitment; frozenStakeTableCommitment = newStakeTableComm; @@ -335,7 +336,12 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { } /// @notice Given the light client state, compute the short commitment of the stake table - function computeStakeTableComm(LightClientState memory state) public pure returns (bytes32) { + function computeStakeTableComm(LightClientState memory state) + public + pure + virtual + returns (bytes32) + { return keccak256( abi.encodePacked( state.stakeTableBlsKeyComm, @@ -349,7 +355,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { /// non-zero address provided /// @dev this function can also be used to update the permissioned prover once it's a different /// address - function setPermissionedProver(address prover) public onlyOwner { + function setPermissionedProver(address prover) public virtual onlyOwner { if (prover == address(0)) { revert InvalidAddress(); } @@ -363,7 +369,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { /// @notice set the permissionedProverMode to false and set the permissionedProver to address(0) /// @dev if it was already disabled (permissioneProverMode == false), then revert with - function disablePermissionedProverMode() public onlyOwner { + function disablePermissionedProverMode() public virtual onlyOwner { if (permissionedProverEnabled) { permissionedProver = address(0); permissionedProverEnabled = false; @@ -419,7 +425,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { } /// @notice get the number of L1 block updates - function getStateUpdateBlockNumbersCount() public view returns (uint256) { + function getStateUpdateBlockNumbersCount() public view virtual returns (uint256) { return stateUpdateBlockNumbers.length; } @@ -429,6 +435,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { function getHotShotCommitment(uint256 hotShotBlockHeight) public view + virtual returns (HotShotCommitment memory) { uint256 commitmentsHeight = hotShotCommitments.length; @@ -447,7 +454,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { } /// @notice get the number of HotShot block commitments - function getHotShotBlockCommitmentsCount() public view returns (uint256) { + function getHotShotBlockCommitmentsCount() public view virtual returns (uint256) { return hotShotCommitments.length; } } diff --git a/contracts/test/LightClientUpgradeToV2.t.sol b/contracts/test/LightClientUpgradeToV2.t.sol deleted file mode 100644 index 4e31d1d8f..000000000 --- a/contracts/test/LightClientUpgradeToV2.t.sol +++ /dev/null @@ -1,94 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.0; -pragma experimental ABIEncoderV2; - -import { Test } /*, console2*/ from "forge-std/Test.sol"; -import { LightClient as LCV1 } from "../src/LightClient.sol"; -import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol"; -import { DeployLightClientContractScript } from "../script/LightClient.s.sol"; -import { UpgradeLightClientScript } from "../script/UpgradeLightClientToV2.s.sol"; - -contract LightClientUpgradeToV2Test is Test { - LCV1 public lcV1Proxy; - LCV2 public lcV2Proxy; - - DeployLightClientContractScript public deployer = new DeployLightClientContractScript(); - UpgradeLightClientScript public upgrader = new UpgradeLightClientScript(); - - LCV1.LightClientState public stateV1; - - address public admin; - address public proxy; - - // deploy the first implementation with its proxy - function setUp() public { - (proxy, admin, stateV1) = deployer.run(10, 5); - lcV1Proxy = LCV1(proxy); - } - - function testCorrectInitialization() public view { - assert(lcV1Proxy.blocksPerEpoch() == 10); - assert(lcV1Proxy.currentEpoch() == 0); - - assertEq(abi.encode(lcV1Proxy.getGenesisState()), abi.encode(stateV1)); - - assertEq(abi.encode(lcV1Proxy.getFinalizedState()), abi.encode(stateV1)); - - bytes32 stakeTableComm = lcV1Proxy.computeStakeTableComm(stateV1); - assertEq(lcV1Proxy.votingStakeTableCommitment(), stakeTableComm); - assertEq(lcV1Proxy.frozenStakeTableCommitment(), stakeTableComm); - assertEq(lcV1Proxy.votingThreshold(), stateV1.threshold); - assertEq(lcV1Proxy.frozenThreshold(), stateV1.threshold); - } - - // that the data remains the same after upgrading the implementation - function testUpgradeSameData() public { - // Upgrade LightClient and check that the genesis state is not changed and that the new - // field - // of the upgraded contract is set to 0 - lcV2Proxy = LCV2(upgrader.run(0, proxy)); - - assertEq(lcV2Proxy.newField(), 0); - assertEq(lcV2Proxy.blocksPerEpoch(), 10); - assertEq(lcV2Proxy.currentEpoch(), 0); - - LCV2.LightClientState memory expectedLightClientState = LCV2.LightClientState( - stateV1.viewNum, - stateV1.blockHeight, - stateV1.blockCommRoot, - stateV1.feeLedgerComm, - stateV1.stakeTableBlsKeyComm, - stateV1.stakeTableSchnorrKeyComm, - stateV1.stakeTableAmountComm, - stateV1.threshold, - 0 // New field for testing purposes - ); - - assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); - } - - // check that the proxy address remains the same - function testUpgradesSameProxyAddress() public { - (uint8 major, uint8 minor, uint8 patch) = lcV1Proxy.getVersion(); - assertEq(major, 1); - assertEq(minor, 0); - assertEq(patch, 0); - - //upgrade box - lcV2Proxy = LCV2(upgrader.run(0, proxy)); - assertEq(address(lcV2Proxy), address(lcV1Proxy)); - (uint8 majorV2, uint8 minorV2, uint8 patchV2) = lcV2Proxy.getVersion(); - assertEq(majorV2, 2); - assertEq(minorV2, 0); - assertEq(patchV2, 0); - } - - function testMaliciousUpgradeFails() public { - address attacker = makeAddr("attacker"); - - //attempted upgrade as attacker will revert - vm.prank(attacker); - vm.expectRevert(); - lcV2Proxy = LCV2(upgrader.run(0, address(proxy))); - } -} diff --git a/contracts/test/LightClientUpgradeToVx.t.sol b/contracts/test/LightClientUpgradeToVx.t.sol new file mode 100644 index 000000000..0dc6c56e2 --- /dev/null +++ b/contracts/test/LightClientUpgradeToVx.t.sol @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; +pragma experimental ABIEncoderV2; + +import { Test } /*, console2*/ from "forge-std/Test.sol"; +import { LightClient as LCV1 } from "../src/LightClient.sol"; +import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol"; +import { LightClientV3 as LCV3 } from "../test/LightClientV3.sol"; +import { DeployLightClientContractScript } from "../script/LightClient.s.sol"; +import { UpgradeLightClientScript } from "./UpgradeLightClientToV2.s.sol"; +import { UpgradeLightClientScript as ULCV3 } from "./UpgradeLightClientToV3.s.sol"; +import { OwnableUpgradeable } from + "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; + +contract LightClientUpgradeToVxTest is Test { + LCV1 public lcV1Proxy; + LCV2 public lcV2Proxy; + LCV3 public lcV3Proxy; + + DeployLightClientContractScript public deployer = new DeployLightClientContractScript(); + UpgradeLightClientScript public upgraderV2 = new UpgradeLightClientScript(); + ULCV3 public upgraderV3 = new ULCV3(); + + LCV1.LightClientState public stateV1; + + address public admin; + address public proxy; + + // deploy the first implementation with its proxy + function setUp() public { + (proxy, admin, stateV1) = deployer.run(10, 5); + lcV1Proxy = LCV1(proxy); + } + + function testCorrectInitialization() public view { + assert(lcV1Proxy.blocksPerEpoch() == 10); + assert(lcV1Proxy.currentEpoch() == 0); + + assertEq(abi.encode(lcV1Proxy.getGenesisState()), abi.encode(stateV1)); + + assertEq(abi.encode(lcV1Proxy.getFinalizedState()), abi.encode(stateV1)); + + bytes32 stakeTableComm = lcV1Proxy.computeStakeTableComm(stateV1); + assertEq(lcV1Proxy.votingStakeTableCommitment(), stakeTableComm); + assertEq(lcV1Proxy.frozenStakeTableCommitment(), stakeTableComm); + assertEq(lcV1Proxy.votingThreshold(), stateV1.threshold); + assertEq(lcV1Proxy.frozenThreshold(), stateV1.threshold); + } + + // test that the data remains the same after upgrading the implementation + function testUpgradeSameDataV1ToV2() public { + // Upgrade LightClient and check that the genesis state is not changed and that the new + // field + // of the upgraded contract is set to 0 + uint256 myNewField = 123; + lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin)); + + assertEq(lcV2Proxy.newField(), myNewField); + assertEq(lcV2Proxy.blocksPerEpoch(), 10); + assertEq(lcV2Proxy.currentEpoch(), 0); + + LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState( + stateV1.viewNum, + stateV1.blockHeight, + stateV1.blockCommRoot, + stateV1.feeLedgerComm, + stateV1.stakeTableBlsKeyComm, + stateV1.stakeTableSchnorrKeyComm, + stateV1.stakeTableAmountComm, + stateV1.threshold + ); + + LCV2.ExtendedLightClientState memory expectedExtendedLightClientState = + LCV2.ExtendedLightClientState(0); + + assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV2Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + } + + // test that the data remains the same after upgrading the implementation + function testExpectRevertUpgradeSameDataV1ToV2ReinitializeTwice() public { + // Upgrade LightClient and check that the genesis state is not changed and that the new + // field + // of the upgraded contract is set to 0 + uint256 myNewField = 123; + lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin)); + + assertEq(lcV2Proxy.newField(), myNewField); + assertEq(lcV2Proxy.blocksPerEpoch(), 10); + assertEq(lcV2Proxy.currentEpoch(), 0); + + LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState( + stateV1.viewNum, + stateV1.blockHeight, + stateV1.blockCommRoot, + stateV1.feeLedgerComm, + stateV1.stakeTableBlsKeyComm, + stateV1.stakeTableSchnorrKeyComm, + stateV1.stakeTableAmountComm, + stateV1.threshold + ); + + LCV2.ExtendedLightClientState memory expectedExtendedLightClientState = + LCV2.ExtendedLightClientState(0); + + assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV2Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + + // expect a revert when we try to reinitialize + vm.expectRevert(Initializable.InvalidInitialization.selector); + lcV2Proxy.initializeV2(5); + } + + // test that the data remains the same after upgrading the implementation + function testUpgradeSameDataV2ToV3() public { + // Upgrade LightClient and check that the genesis state is not changed and that the new + // field + // of the upgraded contract is set to 0 + uint256 myNewField = 123; + uint256 myNewFieldV3 = 456; + lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin)); + + assertEq(lcV2Proxy.newField(), myNewField); + assertEq(lcV2Proxy.blocksPerEpoch(), 10); + assertEq(lcV2Proxy.currentEpoch(), 0); + + LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState( + stateV1.viewNum, + stateV1.blockHeight, + stateV1.blockCommRoot, + stateV1.feeLedgerComm, + stateV1.stakeTableBlsKeyComm, + stateV1.stakeTableSchnorrKeyComm, + stateV1.stakeTableAmountComm, + stateV1.threshold + ); + + LCV2.ExtendedLightClientState memory expectedExtendedLightClientState = + LCV2.ExtendedLightClientState(0); + + assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV2Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + + // upgrade to v3 + lcV3Proxy = LCV3(upgraderV3.run(proxy, myNewFieldV3, admin)); + + assertEq(lcV3Proxy.newField(), myNewField); + assertEq(lcV3Proxy.anotherField(), myNewFieldV3); + assertEq(lcV3Proxy.blocksPerEpoch(), 10); + assertEq(lcV3Proxy.currentEpoch(), 0); + + assertEq(abi.encode(lcV3Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV3Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + } + + // test that the tx reverts if we try to reinitialize more than once + function testExpectRevertUpgradeSameDataV2ToV3ReinitializeTwice() public { + // Upgrade LightClient and check that the genesis state is not changed and that the new + // field + // of the upgraded contract is set to 0 + uint256 myNewField = 123; + uint256 myNewFieldV3 = 456; + // upgrade to v2 first + lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin)); + + assertEq(lcV2Proxy.newField(), myNewField); + assertEq(lcV2Proxy.blocksPerEpoch(), 10); + assertEq(lcV2Proxy.currentEpoch(), 0); + + LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState( + stateV1.viewNum, + stateV1.blockHeight, + stateV1.blockCommRoot, + stateV1.feeLedgerComm, + stateV1.stakeTableBlsKeyComm, + stateV1.stakeTableSchnorrKeyComm, + stateV1.stakeTableAmountComm, + stateV1.threshold + ); + + LCV2.ExtendedLightClientState memory expectedExtendedLightClientState = + LCV2.ExtendedLightClientState(0); + + assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV2Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + + // upgrade to v3 + lcV3Proxy = LCV3(upgraderV3.run(proxy, myNewFieldV3, admin)); + + assertEq(lcV3Proxy.newField(), myNewField); + assertEq(lcV3Proxy.anotherField(), myNewFieldV3); + assertEq(lcV3Proxy.blocksPerEpoch(), 10); + assertEq(lcV3Proxy.currentEpoch(), 0); + + assertEq(abi.encode(lcV3Proxy.getFinalizedState()), abi.encode(expectedLightClientState)); + assertEq( + abi.encode(lcV3Proxy.getExtendedFinalizedState()), + abi.encode(expectedExtendedLightClientState) + ); + + // expect a revert when we try to reinitialize + vm.expectRevert(Initializable.InvalidInitialization.selector); + lcV3Proxy.initializeV3(6); + } + + // check that the proxy address remains the same + function testUpgradesSameProxyAddress() public { + (uint8 major, uint8 minor, uint8 patch) = lcV1Proxy.getVersion(); + assertEq(major, 1); + assertEq(minor, 0); + assertEq(patch, 0); + + //upgrade box + lcV2Proxy = LCV2(upgraderV2.run(proxy, 123, admin)); + assertEq(address(lcV2Proxy), address(lcV1Proxy)); + (uint8 majorV2, uint8 minorV2, uint8 patchV2) = lcV2Proxy.getVersion(); + assertEq(majorV2, 2); + assertEq(minorV2, 0); + assertEq(patchV2, 0); + } + + function testMaliciousUpgradeToV2Fails() public { + address attacker = makeAddr("attacker"); + + //attempted upgrade as attacker will revert + vm.expectRevert( + abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, attacker) + ); + + lcV2Proxy = LCV2(upgraderV2.run(address(proxy), 123, attacker)); + } + + function testMaliciousUpgradeToV32Fails() public { + address attacker = makeAddr("attacker"); + + //attempted upgrade as attacker will revert + + vm.expectRevert( + abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, attacker) + ); + lcV3Proxy = LCV3(upgraderV3.run(address(proxy), 456, attacker)); + } +} diff --git a/contracts/test/LightClientV2.sol b/contracts/test/LightClientV2.sol index ff135f14f..75fe943aa 100644 --- a/contracts/test/LightClientV2.sol +++ b/contracts/test/LightClientV2.sol @@ -3,178 +3,50 @@ pragma solidity ^0.8.0; pragma experimental ABIEncoderV2; -import { OwnableUpgradeable } from - "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; -import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; -import { UUPSUpgradeable } from - "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; - import { BN254 } from "bn254/BN254.sol"; import { IPlonkVerifier } from "../src/interfaces/IPlonkVerifier.sol"; import { PlonkVerifier } from "../src/libraries/PlonkVerifier.sol"; import { LightClientStateUpdateVK as VkLib } from "../src/libraries/LightClientStateUpdateVK.sol"; +import { LightClient } from "../src/LightClient.sol"; /// @notice A light client for HotShot consensus. Keeping track of its finalized states in safe, /// authenticated ways. -contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable { - // === Events === - // - // @notice Notify a new epoch is starting - event EpochChanged(uint64); - - /// @notice upgrade event when the proxy updates the implementation it's pointing to - event Upgrade(address implementation); - - // === Constants === - // - /// @notice System parameter: number of blocks per epoch - /// @dev This variable cannot be made immutable due to how UUPS contracts work. See - /// https://forum.openzeppelin.com/t/upgradable-contracts-instantiating-an-immutable-value/28763/2#why-cant-i-use-immutable-variables-1 - uint32 public blocksPerEpoch; - - /// @notice genesis block commitment index - uint32 internal genesisState; - - /// @notice Finalized HotShot's light client state index - uint32 internal finalizedState; - - // === Storage === - // - /// @notice current (finalized) epoch number - uint64 public currentEpoch; - /// @notice The commitment of the stake table used in current voting (i.e. snapshot at the start - /// of last epoch) - bytes32 public votingStakeTableCommitment; - /// @notice The quorum threshold for the stake table used in current voting - uint256 public votingThreshold; - /// @notice The commitment of the stake table frozen for change (i.e. snapshot at the start of - /// last epoch) - bytes32 public frozenStakeTableCommitment; - /// @notice The quorum threshold for the frozen stake table - uint256 public frozenThreshold; - - /// @notice mapping to store light client states in order to simplify upgrades - mapping(uint32 index => LightClientState value) public states; - - /// @notice the address of the prover that can call the newFinalizedState function when the - /// contract is - /// in permissioned prover mode. This address is address(0) when the contract is not in the - /// permissioned prover mode - address public permissionedProver; - - /// @notice a flag that indicates when a permissioned provrer is needed - bool public permissionedProverEnabled; - - /// @notice an array to store the L1 Block Heights where the finalizedState was updated - uint256[] public stateUpdateBlockNumbers; - - /// @notice an array to store the HotShot Block Heights and their respective HotShot - /// commitments - HotShotCommitment[] public hotShotCommitments; - +contract LightClientV2 is LightClient { /// @notice new field for testing purposes /// @dev In order to add a field to LightClientState struct one can: add a new contract variable /// that has the new struct type, or put the struct inside a map. uint256 public newField; - // === Data Structure === - // - /// @notice The finalized HotShot state (as the digest of the entire HotShot state) - /// @param viewNum The latest view number of the finalized HotShot chain - /// @param blockHeight The block height of the latest finalized block - /// @param blockCommRoot The merkle root of historical block commitments (BN254::ScalarField) - /// @param feeLedgerComm The commitment to the fee ledger state (type: BN254::ScalarField) - /// @param stakeTableBlsKeyComm The commitment to the BlsVerKey column of the stake table - /// @param stakeTableSchnorrKeyComm The commitment to the SchnorrVerKey column of the table - /// @param stakeTableAmountComm The commitment to the stake amount column of the stake table - /// @param threshold The (stake-weighted) quorum threshold for a QC to be considered as valid - struct LightClientState { - uint64 viewNum; - uint64 blockHeight; - BN254.ScalarField blockCommRoot; - BN254.ScalarField feeLedgerComm; - BN254.ScalarField stakeTableBlsKeyComm; - BN254.ScalarField stakeTableSchnorrKeyComm; - BN254.ScalarField stakeTableAmountComm; - uint256 threshold; - uint32 extraField; // New field for testing purposes - } - - /// @notice Simplified HotShot commitment struct - /// @param blockHeight The block height of the latest finalized HotShot block - /// @param blockCommRoot The merkle root of historical block commitments (BN254::ScalarField) - struct HotShotCommitment { - uint64 blockHeight; - BN254.ScalarField blockCommRoot; + struct ExtendedLightClientState { + uint256 extraField; } - /// @notice Event that a new finalized state has been successfully verified and updated - event NewState( - uint64 indexed viewNum, uint64 indexed blockHeight, BN254.ScalarField blockCommRoot - ); - - /// @notice The state is outdated and older than currently known `finalizedState` - error OutdatedState(); - /// @notice Warning that the last block of the current epoch is not yet submitted before newer - /// states are proposed. - error MissingLastBlockForCurrentEpoch(uint64 expectedBlockHeight); - /// @notice Invalid user inputs: wrong format or non-sensible arguments - error InvalidArgs(); - /// @notice Wrong plonk proof or public inputs. - error InvalidProof(); - - /// @notice since the constructor initializes storage on this contract we disable it - /// @dev storage is on the proxy contract since it calls this contract via delegatecall - /// @custom:oz-upgrades-unsafe-allow constructor - constructor() { - _disableInitializers(); + /// @notice mapping to store the extended light client states in order to simplify upgrades + mapping(uint32 index => ExtendedLightClientState state) public extendedStates; + + /// @notice Initialize v2 + /// @param _newField New field amount + /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and + /// is called at most once. The modifier has an uint64 argument which indicates the next + /// contract version. + /// when the base implementation contract is initialized for the first time, the _initialized + /// version + /// is set to 1. Since this is the 2nd implementation, the next contract version is 2. + function initializeV2(uint256 _newField) external reinitializer(2) { + newField = _newField; } /// @notice Use this to get the implementation contract version function getVersion() public pure + virtual + override returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion) { return (2, 0, 0); } - /// @notice only the owner can authorize an upgrade - function _authorizeUpgrade(address newImplementation) internal override onlyOwner { - emit Upgrade(newImplementation); - } - - function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch) - internal - onlyOwner - { - // stake table commitments and threshold cannot be zero, otherwise it's impossible to - // generate valid proof to move finalized state forward. - // Whereas blockCommRoot can be zero, if we use special value zero to denote empty tree. - // feeLedgerComm can be zero, if we optionally support fee ledger yet. - if ( - genesis.viewNum != 0 || genesis.blockHeight != 0 - || BN254.ScalarField.unwrap(genesis.stakeTableBlsKeyComm) == 0 - || BN254.ScalarField.unwrap(genesis.stakeTableSchnorrKeyComm) == 0 - || BN254.ScalarField.unwrap(genesis.stakeTableAmountComm) == 0 || genesis.threshold == 0 - || numBlockPerEpoch == 0 - ) { - revert InvalidArgs(); - } - - states[genesisState] = genesis; - states[finalizedState] = genesis; - currentEpoch = 0; - - blocksPerEpoch = numBlockPerEpoch; - - bytes32 initStakeTableComm = computeStakeTableComm(genesis); - votingStakeTableCommitment = initStakeTableComm; - votingThreshold = genesis.threshold; - frozenStakeTableCommitment = initStakeTableComm; - frozenThreshold = genesis.threshold; - } - // === State Modifying APIs === // /// @notice Update the latest finalized light client state. It is expected to be updated @@ -184,7 +56,7 @@ contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable { function newFinalizedState( LightClientState memory newState, IPlonkVerifier.PlonkProof memory proof - ) external { + ) external virtual override { if ( newState.viewNum <= getFinalizedState().viewNum || newState.blockHeight <= getFinalizedState().blockHeight @@ -217,76 +89,30 @@ contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable { require(newField == 0, "newField can only be set to 0"); // upon successful verification, update the latest finalized state - states[finalizedState] = newState; - emit NewState(newState.viewNum, newState.blockHeight, newState.blockCommRoot); - } + // because newState is in memory and states[finalizedState] is in storage, they have + // different data handling mechanisms + // and this each field needs to be assigned individually + states[finalizedState].viewNum = newState.viewNum; + states[finalizedState].blockHeight = newState.blockHeight; + states[finalizedState].blockCommRoot = newState.blockCommRoot; + states[finalizedState].feeLedgerComm = newState.feeLedgerComm; + states[finalizedState].stakeTableBlsKeyComm = newState.stakeTableBlsKeyComm; + states[finalizedState].stakeTableSchnorrKeyComm = newState.stakeTableSchnorrKeyComm; + states[finalizedState].stakeTableAmountComm = newState.stakeTableAmountComm; + states[finalizedState].threshold = newState.threshold; + + extendedStates[finalizedState].extraField = 2; - /// @dev Simple getter function for the genesis state - function getGenesisState() public view returns (LightClientState memory) { - return states[genesisState]; - } - - /// @dev Simple getter function for the finalized state - function getFinalizedState() public view returns (LightClientState memory) { - return states[finalizedState]; + emit NewState(newState.viewNum, newState.blockHeight, newState.blockCommRoot); } - // === Pure or View-only APIs === - /// @dev Transform a state into an array of field elements, prepared as public inputs of the - /// plonk proof verification - function preparePublicInput(LightClientState memory state) - internal + /// @dev Simple getter function for the extended finalized state + function getExtendedFinalizedState() + public view - returns (uint256[] memory) - { - uint256[] memory publicInput = new uint256[](8); - publicInput[0] = votingThreshold; - publicInput[1] = uint256(state.viewNum); - publicInput[2] = uint256(state.blockHeight); - publicInput[3] = BN254.ScalarField.unwrap(state.blockCommRoot); - publicInput[4] = BN254.ScalarField.unwrap(state.feeLedgerComm); - publicInput[5] = BN254.ScalarField.unwrap(state.stakeTableBlsKeyComm); - publicInput[6] = BN254.ScalarField.unwrap(state.stakeTableSchnorrKeyComm); - publicInput[7] = BN254.ScalarField.unwrap(state.stakeTableAmountComm); - return publicInput; - } - - /// @dev Verify the Plonk proof, marked as `virtual` for easier testing as we can swap VK used - /// in inherited contracts. - function verifyProof(LightClientState memory state, IPlonkVerifier.PlonkProof memory proof) - internal virtual + returns (ExtendedLightClientState memory) { - IPlonkVerifier.VerifyingKey memory vk = VkLib.getVk(); - uint256[] memory publicInput = preparePublicInput(state); - - if (!PlonkVerifier.verify(vk, publicInput, proof)) { - revert InvalidProof(); - } - } - - /// @notice Advance to the next epoch (without any precondition check!) - /// @dev This meant to be invoked only internally after appropriate precondition checks are done - function _advanceEpoch() private { - bytes32 newStakeTableComm = computeStakeTableComm(getFinalizedState()); - votingStakeTableCommitment = frozenStakeTableCommitment; - frozenStakeTableCommitment = newStakeTableComm; - - votingThreshold = frozenThreshold; - frozenThreshold = getFinalizedState().threshold; - - currentEpoch += 1; - emit EpochChanged(currentEpoch); - } - - /// @notice Given the light client state, compute the short commitment of the stake table - function computeStakeTableComm(LightClientState memory state) public pure returns (bytes32) { - return keccak256( - abi.encodePacked( - state.stakeTableBlsKeyComm, - state.stakeTableSchnorrKeyComm, - state.stakeTableAmountComm - ) - ); + return extendedStates[finalizedState]; } } diff --git a/contracts/test/LightClientV3.sol b/contracts/test/LightClientV3.sol new file mode 100644 index 000000000..ffeabc54c --- /dev/null +++ b/contracts/test/LightClientV3.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: UNLICENSED + +pragma solidity ^0.8.0; +pragma experimental ABIEncoderV2; + +import { LightClientV2 } from "./LightClientV2.sol"; + +/// @notice A light client for HotShot consensus. Keeping track of its finalized states in safe, +/// authenticated ways. + +contract LightClientV3 is LightClientV2 { + uint256 public anotherField; + + /// @notice Initialize v3 + /// @param _newField New field amount + /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and + /// is called at most once. The modifier has an uint64 argument which indicates the next + /// contract version. + /// when the base implementation contract is initialized for the first time, the _initialized + /// version + /// is set to 1. Since this is the 3rd implementation, the next contract version is 3. + function initializeV3(uint256 _newField) external reinitializer(3) { + anotherField = _newField; + } + + /// @notice Use this to get the implementation contract version + function getVersion() + public + pure + virtual + override + returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion) + { + return (3, 0, 0); + } +} diff --git a/contracts/script/UpgradeLightClientToV2.s.sol b/contracts/test/UpgradeLightClientToV2.s.sol similarity index 74% rename from contracts/script/UpgradeLightClientToV2.s.sol rename to contracts/test/UpgradeLightClientToV2.s.sol index a60878621..7eb152fcd 100644 --- a/contracts/script/UpgradeLightClientToV2.s.sol +++ b/contracts/test/UpgradeLightClientToV2.s.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; import { Script } from "forge-std/Script.sol"; -import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol"; +import { LightClientV2 as LCV2 } from "./LightClientV2.sol"; import { LightClient as LC } from "../src/LightClient.sol"; contract UpgradeLightClientScript is Script { @@ -11,14 +11,13 @@ contract UpgradeLightClientScript is Script { /// @param mostRecentlyDeployedProxy address of deployed proxy /// @return address of the proxy /// TODO get the most recent deployment from the devops tooling - function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy) + function run(address mostRecentlyDeployedProxy, uint256 newField, address admin) external returns (address) { - string memory seedPhrase = vm.envString("MNEMONIC"); - (address admin,) = deriveRememberKey(seedPhrase, seedPhraseOffset); vm.startBroadcast(admin); - address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV2())); + address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV2()), newField); + vm.stopBroadcast(); return proxy; } @@ -28,14 +27,15 @@ contract UpgradeLightClientScript is Script { /// @param proxyAddress address of proxy /// @param newLightClient address of new implementation /// @return address of the proxy - function upgradeLightClient(address proxyAddress, address newLightClient) + function upgradeLightClient(address proxyAddress, address newLightClient, uint256 newField) public returns (address) { LC proxy = LC(proxyAddress); //make the function call on the previous implementation - proxy.upgradeToAndCall(newLightClient, ""); //proxy address now points to the new + + proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV2.initializeV2, newField)); //proxy + // address now points to the new // implementation - vm.stopBroadcast(); return address(proxy); } } diff --git a/contracts/test/UpgradeLightClientToV3.s.sol b/contracts/test/UpgradeLightClientToV3.s.sol new file mode 100644 index 000000000..3e6158ee9 --- /dev/null +++ b/contracts/test/UpgradeLightClientToV3.s.sol @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +import { Script } from "forge-std/Script.sol"; + +import { LightClientV2 as LCV2 } from "./LightClientV2.sol"; +import { LightClientV3 as LCV3 } from "./LightClientV3.sol"; + +contract UpgradeLightClientScript is Script { + /// @notice runs the upgrade + /// @param mostRecentlyDeployedProxy address of deployed proxy + /// @return address of the proxy + /// TODO get the most recent deployment from the devops tooling + function run(address mostRecentlyDeployedProxy, uint256 newField, address admin) + external + returns (address) + { + vm.startBroadcast(admin); + address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV3()), newField); + vm.stopBroadcast(); + return proxy; + } + + /// @notice upgrades the light client contract by calling the upgrade function the + /// implementation contract via + /// the proxy + /// @param proxyAddress address of proxy + /// @param newLightClient address of new implementation + /// @return address of the proxy + function upgradeLightClient(address proxyAddress, address newLightClient, uint256 newField) + public + returns (address) + { + LCV2 proxy = LCV2(proxyAddress); //make the function call on the previous implementation + + proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV3.initializeV3, newField)); //proxy + // address now points to the new + // implementation + return address(proxy); + } +} diff --git a/doc/smart-contract-upgrades.md b/doc/smart-contract-upgrades.md new file mode 100644 index 000000000..455807392 --- /dev/null +++ b/doc/smart-contract-upgrades.md @@ -0,0 +1,304 @@ +# Smart Contract Upgrades + +Code on the EVM is immutable but via proxies, smart contracts can be “upgraded”. A proxy contract acts as an +intermediary that can delegate calls to other contracts, called implementation contracts. This allows the logic of a +smart contract to be upgraded while keeping the same address and state. This is made possible by two functions in +Solidity: + +- `delegatecall`: ContractA can delegatecall ContractB but execute the logic within the context of ContractA. This means + that the called contract's code is executed with the storage, caller, and balance of the calling contract. +- `fallback`: executed when a function does not exist. This is often used in proxy contracts to catch all function calls + and delegate them to the implementation contract. + +OpenZeppelin's [implementation](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) for the UUPS +proxy pattern was used. + +At the time of writing, we have two upgradable smart contracts: + +- LightClient.sol +- FeeContract.sol + +They both use OpenZeppelin's [implementation](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) for +the UUPS proxy pattern and is deployed using OpenZeppelin Upgrades library. + +## Writing Upgradable Smart Contracts + +This document is intended for developers to explain the process for upgrading contracts within our repo to ensure safe +upgrades. + +### Upgrading via Contract Inheritance + +When writing the upgrade for upgradable contracts such as the light client contract and the fee contract, the new +implementation inherits from the latest implementation contract. This new implementation only implements the new or +modified features or new variables. If any variables have to be initialized, the new implementation contract should +create a `initializeVx` function, where x is the version number. + +The `reinitializer` modifier, provided by the +[`Initializable`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol) +library, ensures that the new implementation is reinitialized at most once. The modifier takes a `uint64` argument, +which specifies the contract's version. + +When the base implementation contract is initialized, its `_initializedVersion` is set to 1. The `reinitializer` +modifier ensures that the initialization logic is only executed if the \_initializedVersion is less than the specified +version number. Therefore, when upgrading a contract, the `reinitializer` modifier should be set to the previous +`_initializedVersion` plus 1 to indicate the new version. + +Here's an example: + +The original `LightClient` contract looks like this: + +```solidity +contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable { + //... +} +``` + +The 2nd implementation, LightClientV2, that adds a new variable is implemented like this: + +```solidity +contract LightClientV2 is LightClient { + uint256 public newField; + + /// @notice Initialize v2 + /// @param _newField New field amount + /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and + /// is called at most once. The modifier has an uint64 argument which indicates the next contract version. + /// when the base implementation contract is initialized for the first time, the _initialized version + /// is set to 1. Since this is the 2nd implementation, the next contract version is 2. + function initializeV2(uint256 _newField) external reinitializer(2){ + newField = _newField; + } + + function newFinalizedState( + LightClientState memory newState, + IPlonkVerifier.PlonkProof memory proof + ) external virtual override { + //... + } + + //... +} +``` + +The third implementation, LightClientV3, will inherit from LightClientV2. Ensure that functions are marked as `virtual` +in previous versions of the contract if you intend to override them in child contracts. + +```solidity +contract LightClientV3 is LightClientV2 { + uint256 public anotherField; + + /// @param _newField New field amount + function initializeV3(uint256 _newField) external reinitializer(3){ + anotherField = _newField; + } + + function newFinalizedState( + LightClientState memory newState, + IPlonkVerifier.PlonkProof memory proof + ) external virtual override { + //... + } + + //... +} +``` + +Solidity supports inheritance which means that a function call always executes the function of the same name (and +parameter types) in the most derived contract in the inheritance hierarchy. + +**Pros**: + +- **Modularity**: each contract version can focus on specific features or upgrades +- **Reusability:** code doesn’t have to be duplicated in newer versions which reduces the likelihood of errors +- **Ease of extension:** easier to extend features without worrying about the rest of the implementation +- **C3 linearization**: C3 linearization provides a deterministic way to determine the storage layout of a contract + based on its inheritance hierarchy. This means that if we maintain the same inheritance order in subsequent contract + versions, the storage layout will remain consistent, even if we add new state variables. + +**Cons**: + +- **Deployment size:** Contracts that inherit from multiple base contracts can become large, potentially exceeding the + maximum contract size limit on the Ethereum blockchain +- **State variable shadowing:** When a new version of a contract inherits from an older version, introducing a state + variable with the same name as one in the base contract will lead to a compilation error due to shadowing. Therefore, + one cannot simply redefine an existing struct with new members if inheritance is used for upgrades. This can become + cumbersome in large projects with many inherited contracts +- **Multiple inheritance:** Solidity’s C3 linearization predicts the order in which base contracts are inherited, and + their functions are called. So the order inherited is the order they will be in the storage layout and this needs to + be taken under consideration. When multiple base contracts define state variables, the order of inheritance can affect + the storage layout. Mismanaging this can lead to overlapping storage slots, causing data corruption. + +### Upgrades supported + +- new state variables +- new logic +- modifying existing logic (function overriding) + +#### New State Variables + +Thanks to C3 linearization, the order in which you inherit from the previous version of the contract dictates the +storage layout so be sure to be mindful when declaring new variables. + +Criteria for New State Variables: + +- New variable names must be used otherwise it leads to a compilation error due to shadowing. +- New variables added _must_ be declared as type `internal` or `public`, `private` can never be used. +- If the new variable needs to be initialized, then the initialize function with a separate initV[X] function should be + added. The initializeVx function allows the contract to initialize only the new variables added in the new contract. + +_Example:_ + +```solidity +// Version 1 +contract V1 { + uint public oldValue; + uint8 internal _initializedVersion + + function initialize(uint _oldValue) external { + require(_initializedVersion == 0); + oldValue = _oldValue; + _initializedVersion = 1; + } +} + +// Version 2 +contract V2 is V1 { + uint public newValue; + + function initializeV2(uint _newValue) external { + require(_initializedVersion == 1); + newValue = _newValue; + _initializedVersion = 2; + } +} +``` + +#### New Logic + +Base functions can be overridden by inheriting contracts to change their behavior if they are marked as virtual. Thus if +you plan to keep a function upgradable, always mark it as virtual in new versions of the contract. + +_Example:_ + +```solidity +// Version 1 +contract V1 { + function getValue() public view virtual returns (uint) { + return 1; + } +} + +// Version 2 +contract V2 is V1 { + function getValue() public view virtual override returns (uint) { + return 2; + } +} +``` + +#### Modifying Existing Logic (Function Overriding) + +The logic in existing functions can be overridden but be sure not to introduce breaking changes. + +Criteria for Overriding Functions:: + +- a function must be marked as `virtual` in the base contract so that it can be eligible to be overridden in a derived + contract. +- a function with `private` visibility cannot be overridden or inherited. +- Public state variables can override `external` functions if the parameter and return types of the function matches the + getter function of the variable: + +_Example:_ + +```solidity +// SPDX-License-Identifier: GPL-3.0 +pragma solidity >=0.6.0 <0.9.0; + +contract A { + function getValue() public view virtual returns (uint) { + return 1; + } +} + +contract B is A { + function getValue() public view virtual override returns (uint) { + return 2; + } +} +``` + +- The overriding function may only change the visibility of the overridden function from `external` to `public`. +- The mutability may be changed to a more strict one following the order: `nonpayable` can be overridden by `view` and + `pure`. `view` can be overridden by `pure`. `payable` is an exception and cannot be changed to any other mutability. + +##### Avoiding breaking changes when overriding functions + +- When overriding a function, consider whether it’s necessary to call the parent contract’s implementation using + `super`. This can preserve the original logic as you extend with new functionality. +- Ensure that any side effects are handled appropriately, e.g. event emissions, state variable modifications etc + +### Upgrades that become complicated through inheritance + +#### Upgrading Structs + +Unfortunately, directly upgrading structs through inheritance is not straightforward due to Solidity's storage layout +rules. When a new version of a contract inherits from an older version, introducing a state variable with the same name +as one in the base contract will lead to a compilation error due to shadowing. + +**Potential workarounds:** + +- Additional struct: create a new struct with additional fields that extends the original struct so that both can be + used in tandem. +- New Struct: Create a new struct with additional fields and migrate existing data to the new struct during the upgrade + process. _Example:_ + +```solidity + +// Version 1 +contract V1 { + struct Data { + uint value; + } + + Data public data; +} + +// Version 2 +contract V2 is V1 { + struct NewData { + uint value; + uint newValue; + } + + NewData public newData; + + function migrateData() public { + newData = NewData(data.value, 0); + } +} +``` + +## Deploying the Upgradable Contract + +The [OpenZeppelin Upgrade plugins](https://docs.openzeppelin.com/upgrades-plugins/1.x/) are used to assist with upgrades +so that we can ensure that the base and future implementation contracts are upgrade safe. For example, the +implementation contract: + +- cannot have a constructor. +- should not use the `selfdestruct` or `delegatecall`. +- should not initialize state variables. +- should not make state variables `immutable`. + +You can learn more about deploying and upgrading upgradable contracts with OpenZeppelin and Foundry +[here](https://docs.openzeppelin.com/upgrades-plugins/1.x/foundry-upgrades). + +### Upgrading via the Proxy + +- Deploy the new (modified) implementation contract. If new state variables that need initialization were added then: +- Prepare an `upgradeToAndCall` transaction from the multisig admin account which is parameterized with the address of + the new implementation contract and an internal call to invoke initializeVx with the new state variables data. Else: +- Prepare an `upgradeTo` transaction from the multisig admin account parameterized with the address of the new + implementation contract: Then: +- Broadcast the transaction + +For more details, check out this [README](../contracts/script/README.md).