Deploy script review

[nikitastupin-matterlabs]

Hi! Thank you for writing the deploy script. I've reviewed it and sharing my comments and questions below. Let me know if you have any doubts!

zkstaker/script/DeployZkStaker.ts

Line 60 in 692d3e7

const mintRewardNotifier = await deployer.deploy(mintRewardNotifierContractArtifact, [zkStakerContractAddress, REWARD_AMOUNT, REWARD_INTERVAL, ZK_TOKEN_TIMELOCK_ADDRESS, ZK_TOKEN_TIMELOCK_ADDRESS], "create", undefined);

The 5th parameter is minter which is set to the timelock address. However, the proposal says that it should be a CappedMinter.

zkstaker/src/ZkStaker.sol

Line 24 in 692d3e7

contract ZkStaker is Staker, StakerPermitAndStake, StakerOnBehalf, StakerDelegateSurrogateVotes {

The ZkStaker contract doesn't inherit from StakerCapDeposits which is contrary to the proposal.

zkstaker/script/DeployZkStaker.ts

Line 22 in 692d3e7

const ZK_TOKEN_TIMELOCK_ADDRESS = "0x3E21c654B545Bf6236DC08236169DcF13dA4dDd6"; // TDDO: Verify this address

The timelock address is 0xC3e970cB015B5FC36edDf293D2370ef5D00F7a19 according to the proposal. The same address returned by the ZkGovOpsGovernor contract.

Would it also make sense to rename ZK_TOKEN_TIMELOCK_ADDRESS to ZK_GOV_OPS_TIMELOCK_ADDRESS?

zkstaker/script/DeployZkStaker.ts

Lines 30 to 31 in 692d3e7

	// TODO: Uncomment this line referencing the environment variable when secret can be set on CI
	const deployerPrivateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"

Would it make sense to resolve the TODO?

zkstaker/README.md

Lines 1 to 2 in 692d3e7

	# ScopeLift Foundry Template

It'd be great to add how to use the deploy script. What variables should be configured? What files should be configured (e.g. .env)? What are the commands to perform the deployment?

zkstaker/script/DeployZkStaker.ts

Line 23 in 692d3e7

const MAX_BUMP_TIP = "1000000000000000000"; // 1e18 string instead of bigNumber

Should it be 0 in the ZkStaker case? We've discussed it with Rafael.

zkstaker/script/DeployZkStaker.ts

Line 72 in 692d3e7

console.log(`ZKSTAKER_ADDRESS=${zkStakerContractAddress}\nEARNING_POWER_CALCULATOR_ADDRESS=${earningPowerCalculaterContractAddress}\nMINT_REWARD_NOTIFIER_ADDRESS=${mintRewardNotifierContractAddress}\n`);

I'm not sure how it's usually done but is it possible to verify the contracts in the same deployment script?

[alexkeating]

Thanks for the review @nikitastupin-matterlabs!

The 5th parameter is minter which is set to the timelock address. However, the proposal says that it should be a CappedMinter.

• Yes we will make this change.

The ZkStaker contract doesn't inherit from StakerCapDeposits which is contrary to the proposal.

Yes definitely, we were working on a pr here during the review.

Thee timelock address is 0xC3e970cB015B5FC36edDf293D2370ef5D00F7a19 according to the proposal. The same address returned by the ZkGovOpsGovernor contract.

Definitely!

Would it make sense to resolve the TODO?

We are planning to update the readme to reflect the repo.

It'd be great to add how to use the deploy script. What variables should be configured? What files should be configured (e.g. .env)? What are the commands to perform the deployment?

Yep this will be in the readme.

Should it be 0 in the ZkStaker case? We've discussed it with Rafael.

Yep we can make it 0. I will note the notion document mentions a non-zero tip.

I'm not sure how it's usually done but is it possible to verify the contracts in the same deployment script?

We typically verify using hardhat in a separate command after deployment.

[nikitastupin-matterlabs]

Hi @alexkeating!

Yep we can make it 0. I will note the notion document mentions a non-zero tip.

That's true, the doc states it should be non-zero. The logic behind setting it to zero is that the IdentityEarningPowerCalculator, which we use, returns the same earning power so the bumpEarningPower function always reverts here. Thus it doesn't really matter what the value of maxBumpTip. The zero value might preferable because it signals that there's no incentive to call the IdentityEarningPowerCalculator (since it always reverts).

I'd appreciate If you could let me know once the changes are applied. I could use the deploy scripts to deploy to the testnet and check how it works there then.

Thank you!

[alexkeating]

Good point! I think it would revert here though as the earning power will not change between old and new.

Also, quick question, do you have the address for the capped minter?

[nikitastupin-matterlabs]

Also, quick question, do you have the address for the capped minter?

As far as I see it should be a new capped minter and the spec mentions that Tally deploys one. Reading the spec, the source code should be https://github.com/zksync-association/zk-governance/blob/master/l2-contracts/src/ZkCappedMinterV2.sol (but it's better to double-check this with someone else from the team).

I've also noticed the spec mentions that the admin of the Staker contract, initially, should be some multi-sig while in other places it says it should be the GovOps timelock. I think it's also good to double-check that.

To provide more context, I'm only helping with the security side of things and, unfortunately, not aware of the whole context of the agreement and all the details. That's why you may see some uncertainties from my side.

[alexkeating]

Cool, we are still figuring out the paraemters, and will deploy the minters from the v2 factory located at 0x0400E6bc22B68686Fb197E91f66E199C6b0DDD6a. We will let you know when the scripts are ready for another security review.

[nikitastupin-matterlabs]

I've checked the most recent version, in general looks good to me but a few nits below! These aren't blockers but will make life easier:

zkstaker/README.md

Lines 162 to 164 in d3af27d

	```bash
	npx hardhat run script/DeployZkStaker.ts --network zkSyncEraTestnet
	```

zkSyncEraTestnet should be zkSyncTestnet

zkstaker/script/DeployZkStaker.ts

Lines 32 to 34 in d3af27d

	// TODO: Uncomment this line referencing the environment variable when secret can be set on CI
	const deployerPrivateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"
	// const deployerPrivateKey = process.env.DEPLOYER_PRIVATE_KEY;

The TODO might be resolved

[nikitastupin-matterlabs]

One more thing that I'm still debugging. I've deployed the contracts to the zkSync Sepolia but I cannot stake for some reason. It reverts without a clear error message. I've tried both via sepolia.explorer.zksync.io + MetaMask and via cast.

cast call --trace --private-key $PK 0xd8D1316971995ffbc88f5Afe0EDe9AC1aD27A43C 'stake(uint256,address)' 1 0x7D9Af1fF2a96b52d360d6FbB1B91Dcf1B59AAB21

Traces:
  [0] 0xd8D1316971995ffbc88f5Afe0EDe9AC1aD27A43C::stake(1, 0x7D9Af1fF2a96b52d360d6FbB1B91Dcf1B59AAB21)
    └─ ← [Stop] 

If you have any hint please let me know!

Update: I've forked the testnet and run it in Foundry and it turned out that a surrogate calls delegate on the token but my token doesn't have the delegate function 🤦

Update: I've interacted with the contract on the testnet and things work. Good from my side.