-
Notifications
You must be signed in to change notification settings - Fork 387
VERY DRAFT Multisig verifier #1643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: draft-v29.5
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity 0.8.28; | ||
|
||
import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable-v4/access/Ownable2StepUpgradeable.sol"; | ||
Check failure on line 4 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {LibMap} from "./libraries/LibMap.sol"; | ||
Check failure on line 5 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {IZKChain} from "./chain-interfaces/IZKChain.sol"; | ||
Check failure on line 6 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {TimeNotReached, NotAZKChain} from "../common/L1ContractErrors.sol"; | ||
Check failure on line 7 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {IBridgehub} from "../bridgehub/IBridgehub.sol"; | ||
Check failure on line 8 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {IValidatorTimelock} from "./IValidatorTimelock.sol"; | ||
Check failure on line 9 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
import {IExecutor} from "./chain-interfaces/IExecutor.sol"; | ||
import {ValidatorTimelock} from "./ValidatorTimelock.sol"; | ||
import {EIP712Upgradeable} from "@openzeppelin/contracts-upgradeable-v4/utils/cryptography/EIP712Upgradeable.sol"; | ||
import {SignatureCheckerUpgradeable} from "@openzeppelin/contracts-upgradeable-v4/utils/cryptography/SignatureCheckerUpgradeable.sol"; | ||
|
||
|
||
//TODO proper errors | ||
//TODO maybe combine with ValidatorTimelock | ||
contract MultisigCommiter is ValidatorTimelock, EIP712Upgradeable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a new name for this contract (I mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
/// @dev EIP-712 TypeHash for commitBatchesMultisig | ||
bytes32 internal constant COMMIT_BATCHES_MULTISIG_TYPEHASH = | ||
keccak256("CommitBatchesMultisig(address chainAddress, uint256 processBatchFrom, uint256 processBatchTo, bytes batchData)"); | ||
|
||
bytes32 internal constant COMMIT_VERIFIER_ROLE = keccak256("COMMIT_VERIFIER_ROLE"); | ||
|
||
mapping(address chainAddress => uint256 signingThreshold) internal signingThreshold; | ||
|
||
constructor(address _bridgehubAddr) ValidatorTimelock(_bridgehubAddr) {} | ||
|
||
//TODO __EIP712_init | ||
|
||
function commitBatchesSharedBridge( | ||
address _chainAddress, | ||
uint256 _processBatchFrom, | ||
uint256 _processBatchTo, | ||
bytes calldata _batchData | ||
Check failure on line 35 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
) external override { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no access control |
||
require(signingThreshold[_chainAddress] == 0, "Chain requires verifiers signatures for commit"); | ||
Check failure on line 37 in l1-contracts/contracts/state-transition/MultisigCommiter.sol
|
||
_recordBatchCommitment(_chainAddress, _processBatchFrom, _processBatchTo); | ||
_propagateToZKChain(_chainAddress); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it would be nice if you used inheritance here i.e. invoked |
||
} | ||
|
||
function commitBatchesMultisig( | ||
address chainAddress, | ||
uint256 _processBatchFrom, | ||
uint256 _processBatchTo, | ||
bytes calldata _batchData, | ||
address[] calldata signers, | ||
bytes[] calldata signatures | ||
) external onlyRole(chainAddress, COMMITTER_ROLE) { | ||
bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(COMMIT_BATCHES_MULTISIG_TYPEHASH, chainAddress, _processBatchFrom, _processBatchTo, _batchData))); | ||
|
||
_checkSignatures(chainAddress, signers, signatures, digest); | ||
|
||
_recordBatchCommitment(chainAddress, _processBatchFrom, _processBatchTo); | ||
// we cannot use _propagateToZKChain here, becouse function signature is altered | ||
IExecutor(chainAddress).commitBatchesSharedBridge(chainAddress, _processBatchFrom, _processBatchTo, _batchData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is a bit unfortunate that we do a full ABI-encoding here. It will come at a cost, but hopefully it wont be too much:
|
||
} | ||
|
||
function _checkSignatures(address chainAddress, address[] calldata signers, bytes[] calldata signatures, bytes32 digest) internal view { | ||
require(signers.length == signatures.length, "Mismatching signatures length"); | ||
require(signers.length >= signingThreshold[chainAddress], "Not enough signers"); | ||
|
||
// signers must be sorted in order to cheaply validate they are not duplicated | ||
address previousSigner = address(0); | ||
for (uint256 i = 0; i < signers.length; i++) { | ||
require(signers[i] > previousSigner, "Signers must be sorted"); | ||
require(hasRole(chainAddress, COMMIT_VERIFIER_ROLE, signers[i]), "Invalid signature"); | ||
require(SignatureCheckerUpgradeable.isValidSignatureNow(signers[i], digest, signatures[i]), "Invalid signature"); | ||
previousSigner = signers[i]; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point to discuss. In my opinion, unless there's a reason to have two separate contracts (i.e.
MultisigCommiter
andValidatorTimelock
) I'd do one. Having differentValidatorTimelock
s on different CTMs will make things more complex (e.g. explaining people why we have two contracts; but also keeping track which CTM has which contract).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if a CTM owner wants to use the 2FA feature at some point it's easy to turn on in case the feature is already there (vs upgrading the implementation).