EspressoSystems / espresso-sequencer

102 stars 69 forks source link

Simplify light client contract for audit #1912

Open alxiong opened 2 months ago

alxiong commented 2 months ago

Current light client contracts contains logic related to epoch change and stake table commitment updates, which won't be needed for our launch since we assume a static stake table.

We want to remove unused logic both for audit and for lighter weight deployment at launch, here are the proposed changes:

StakeStake genesisStakeTableState;


-  remove the states mapping and  add a `genesisState` variable and add a `finalizedState` variable 
```solidity
LightClientState genesisState;
LightClientState lightClientState;
alxiong commented 2 months ago

our current LC impl stores this:

    /// @notice genesis block commitment index
    uint32 internal genesisState;
    /// @notice Finalized HotShot's light client state index
    uint32 internal finalizedState;

    mapping(uint32 index => LightClientState value) public states;

in the hope that if our struct LightClientState gets updated in the future (e.g. extra member fields), and to work with the proxy storage slot in the proxy, we choose to put them behind a mapping.

I find our design strange:


Besides, more questions:

alysiahuggins commented 2 months ago

for the first bullet point mark threshold, 3 stakeTableXXXComm as immutable global variables, defined at initialize() you can't have immutable variables in an upgradeable contract. This is because immutable variables are set in the constructor which uses the implementation contract's storage. However, the proxy pattern used for upgradeability requires storage be on the proxy and thus variables are initialized via a method named initialize. The alternative is to declare them as regular stare variables and assign a value in the initialize method.

alysiahuggins commented 2 months ago
  • storage

our current LC impl stores this:

    /// @notice genesis block commitment index
    uint32 internal genesisState;
    /// @notice Finalized HotShot's light client state index
    uint32 internal finalizedState;

    mapping(uint32 index => LightClientState value) public states;

in the hope that if our struct LightClientState gets updated in the future (e.g. extra member fields), and to work with the proxy storage slot in the proxy, we choose to put them behind a mapping.

I find our design strange:

  • we shouldn't ever change the genesis once we go into production (if the chain really gets redeployed, so does the LC contract) -- the type for the finalized state might change, but that for the genesis (or any history for that matter) wouldn't.
  • we need to ensure the immutability of the genesis state across implementations (which i don't think this design achieves that?)

Besides, more questions:

  • why did we include feeLedgerComm in the LightClientState again? isn't the root of fee ledger MT already part of the BlockHeader (see here), thus accumulated in the blcokCommRoot? Can we just remove this field?
  • if so, do we really expect the struct LightClientState to change? maybe all the change would be abosrbed in the change of BlockHeader, but at the LC contract level, we only deal with the block commitment MT root, which is oblivious to these changes?

    • one obvious foresight is the upcoming dynamic stake table: the table commitments will be changed and the table schema itself can be updated (e.g. removal of Schnorr PubKey). For this, we can simply declare them as contract variable bytes stakeTableCommtiments = abi.encode(...), and if the stake table commitment computation changed, we just change the way we parse it: abi.decode(stakeTableCommitments, (uint256, ..., NewStructCol));

Since we don't plan to change the genesis state, i agree that there is less need for the states mapping

alysiahuggins commented 2 months ago

for the first bullet point mark threshold, 3 stakeTableXXXComm as immutable global variables, defined at initialize() you can't have immutable variables in an upgradeable contract. This is because immutable variables are set in the constructor which uses the implementation contract's storage. However, the proxy pattern used for upgradeability requires storage be on the proxy and thus variables are initialized via a method named initialize. The alternative is to declare them as regular stare variables and assign a value in the initialize method.

Alex shared this https://forum.openzeppelin.com/t/un-safe-usage-of-immutable-in-uups/28783 which discusses the use of immutable variables - it's safe to use once you're ok with all proxies pointing to the same value

Also one would need to add unsafeAllow: ['state-variable-immutable'] so that openzeppelin doesn't complain when deploying / upgrading it