From f208098b2e922168c2d7be3bc802e9098160680a Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Wed, 15 Nov 2023 17:00:44 -0800 Subject: [PATCH 1/6] ethereum: Add verifyCurrentQuorum method to core bridge --- ethereum/contracts/Messages.sol | 40 ++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index ec6b43a514..b40008b61e 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -147,7 +147,7 @@ contract Messages is Getters { /** - * @dev verifySignatures serves to validate arbitrary sigatures against an arbitrary guardianSet + * @dev verifySignatures serves to validate arbitrary signatures against an arbitrary guardianSet * - it intentionally does not solve for expectations within guardianSet (you should use verifyVM if you need these protections) * - it intentioanlly does not solve for quorum (you should use verifyVM if you need these protections) * - it intentionally returns true when signatures is an empty set (you should use verifyVM if you need these protections) @@ -187,6 +187,44 @@ contract Messages is Getters { return (true, ""); } + /** + * @dev verifyCurrentQuorum serves to validate arbitrary signatures and check quorum against the current guardianSet + */ + function verifyCurrentQuorum(bytes32 hash, Structs.Signature[] memory signatures) public view returns (bool valid, string memory response) { + uint32 gsi = getCurrentGuardianSetIndex(); + Structs.GuardianSet memory guardianSet = getGuardianSet(gsi); + + /** + * @dev Checks whether the guardianSet has zero keys + * WARNING: This keys check is critical to ensure the guardianSet has keys present AND to ensure + * that guardianSet key size doesn't fall to zero and negatively impact quorum assessment. If guardianSet + * key length is 0 and vm.signatures length is 0, this could compromise the integrity of both vm and + * signature verification. + */ + if(guardianSet.keys.length == 0){ + return (false, "invalid guardian set"); + } + + /** + * @dev We're using a fixed point number transformation with 1 decimal to deal with rounding. + * WARNING: This quorum check is critical to assessing whether we have enough Guardian signatures to validate a VM + * if making any changes to this, obtain additional peer review. If guardianSet key length is 0 and + * vm.signatures length is 0, this could compromise the integrity of both vm and signature verification. + */ + if (signatures.length < quorum(guardianSet.keys.length)){ + return (false, "no quorum"); + } + + /// @dev Verify the proposed vm.signatures against the guardianSet + (bool signaturesValid, string memory invalidReason) = verifySignatures(hash, signatures, guardianSet); + if(!signaturesValid){ + return (false, invalidReason); + } + + /// If we are here, we've validated the VM is a valid multi-sig that matches the current guardianSet. + return (true, ""); + } + /** * @dev parseVM serves to parse an encodedVM into a vm struct * - it intentionally performs no validation functions, it simply parses raw into a struct From 58865be1bf28f54c5cb98f2110438489e6ae0d04 Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Thu, 16 Nov 2023 09:38:12 -0800 Subject: [PATCH 2/6] ethereum: Add tests for verifyCurrentQuorum --- ethereum/contracts/interfaces/IWormhole.sol | 2 + ethereum/forge-test/Messages.t.sol | 132 ++++++++++++++++++ .../forge-test/relayer/MockWormhole.sol | 7 + 3 files changed, 141 insertions(+) diff --git a/ethereum/contracts/interfaces/IWormhole.sol b/ethereum/contracts/interfaces/IWormhole.sol index 3d68bc8c68..0ab4d05b63 100644 --- a/ethereum/contracts/interfaces/IWormhole.sol +++ b/ethereum/contracts/interfaces/IWormhole.sol @@ -100,6 +100,8 @@ interface IWormhole { function verifySignatures(bytes32 hash, Signature[] memory signatures, GuardianSet memory guardianSet) external pure returns (bool valid, string memory reason); + function verifyCurrentQuorum(bytes32 hash, Signature[] memory signatures) external view returns (bool valid, string memory response); + function parseVM(bytes memory encodedVM) external pure returns (VM memory vm); function quorum(uint numGuardians) external pure returns (uint numSignaturesRequiredForQuorum); diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index 4cb8e7a19b..a5c3460ba1 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -330,4 +330,136 @@ contract TestMessages is Test { assertEq(vm_.signatures[i].v, vmOptimized.signatures[i].v); } } + /// @dev This test checks that quorum is reached in the happy path + function testFuzz_verifyCurrentQuorum(bytes memory encoded) public { + vm.assume(encoded.length > 0); + + // Set the initial guardian set + address[] memory initialGuardians = new address[](1); + initialGuardians[0] = testGuardianPub; + + // Create a guardian set + Structs.GuardianSet memory initialGuardianSet = Structs.GuardianSet({ + keys: initialGuardians, + expirationTime: 0 + }); + + messages.storeGuardianSetPub(initialGuardianSet, uint32(0)); + + bytes32 message = keccak256(encoded); + + // Generate legitimate signature. + Structs.Signature memory goodSignature; + (goodSignature.v, goodSignature.r, goodSignature.s) = vm.sign(testGuardian, message); + assertEq(ecrecover(message, goodSignature.v, goodSignature.r, goodSignature.s), vm.addr(testGuardian)); + goodSignature.guardianIndex = 0; + + // Attempt to verify signatures. + Structs.Signature[] memory sigs = new Structs.Signature[](1); + sigs[0] = goodSignature; + + (bool valid, string memory reason) = messages.verifyCurrentQuorum(message, sigs); + assertEq(valid, true); + assertEq(bytes(reason).length, 0); + } + + /// @dev This test checks that quorum will not be reached if there are no guardians in the current guardian set + function testFuzz_verifyCurrentQuorum_noGuardians(bytes memory encoded) public { + vm.assume(encoded.length > 0); + + bytes32 message = keccak256(encoded); + + // Generate legitimate signature. + Structs.Signature memory goodSignature; + (goodSignature.v, goodSignature.r, goodSignature.s) = vm.sign(testGuardian, message); + assertEq(ecrecover(message, goodSignature.v, goodSignature.r, goodSignature.s), vm.addr(testGuardian)); + goodSignature.guardianIndex = 0; + + // Attempt to verify signatures. + Structs.Signature[] memory sigs = new Structs.Signature[](1); + sigs[0] = goodSignature; + + (bool valid, string memory reason) = messages.verifyCurrentQuorum(message, sigs); + assertEq(valid, false); + assertEq(abi.encodePacked(reason), abi.encodePacked("invalid guardian set")); + } + + /// @dev This test checks that quorum will not be reached if there is >0 invalid signatures in any position + function testFuzz_verifyCurrentQuorum_invalidSignature(bytes memory encoded, uint8 numSigs, uint256 invalidSigIndex) public { + vm.assume(encoded.length > 0); + vm.assume(numSigs != 0); + invalidSigIndex = bound(invalidSigIndex, 0, numSigs - 1); + + // Set the initial guardian set + address[] memory initialGuardians = new address[](numSigs); + + bytes32 message = keccak256(encoded); + + Structs.Signature[] memory signatures = new Structs.Signature[](numSigs); + + for (uint8 i = 0; i < numSigs; ++i) { + (address addr, uint256 key) = makeAddrAndKey(string(abi.encode(i))); + initialGuardians[i] = addr; + + // Create a single invalid signature + if (i == invalidSigIndex) { + (addr, key) = makeAddrAndKey(string(abi.encode(type(uint64).max))); + (signatures[i].v, signatures[i].r, signatures[i].s) = vm.sign(key, message); + signatures[i].guardianIndex = i; + continue; + } + + (signatures[i].v, signatures[i].r, signatures[i].s) = vm.sign(key, message); + signatures[i].guardianIndex = i; + } + + // Create a guardian set + Structs.GuardianSet memory initialGuardianSet = Structs.GuardianSet({ + keys: initialGuardians, + expirationTime: 0 + }); + + messages.storeGuardianSetPub(initialGuardianSet, uint32(0)); + + (bool valid, string memory reason) = messages.verifyCurrentQuorum(message, signatures); + assertEq(valid, false); + assertEq(abi.encodePacked(reason), abi.encodePacked("VM signature invalid")); + } + + /// @dev This test checks that quorum will not be reached with 0); + numGuardians = bound(numGuardians, 1, type(uint8).max); + numSigs = bound(numSigs, 0, messages.quorum(numGuardians) - 1); + + // Set the initial guardian set + address[] memory initialGuardians = new address[](numGuardians); + + bytes32 message = keccak256(encoded); + + Structs.Signature[] memory signatures = new Structs.Signature[](numSigs); + + for (uint8 i = 0; i < numGuardians; ++i) { + (address addr, uint256 key) = makeAddrAndKey(string(abi.encode(i))); + initialGuardians[i] = addr; + + if (i < numSigs) { + (signatures[i].v, signatures[i].r, signatures[i].s) = vm.sign(key, message); + signatures[i].guardianIndex = i; + } + } + + // Create a guardian set + Structs.GuardianSet memory initialGuardianSet = Structs.GuardianSet({ + keys: initialGuardians, + expirationTime: 0 + }); + + messages.storeGuardianSetPub(initialGuardianSet, uint32(0)); + + (bool valid, string memory reason) = messages.verifyCurrentQuorum(message, signatures); + assertEq(valid, false); + assertEq(abi.encodePacked(reason), abi.encodePacked("no quorum")); + } + } diff --git a/relayer/ethereum/forge-test/relayer/MockWormhole.sol b/relayer/ethereum/forge-test/relayer/MockWormhole.sol index 16ef6d6514..98059915a2 100644 --- a/relayer/ethereum/forge-test/relayer/MockWormhole.sol +++ b/relayer/ethereum/forge-test/relayer/MockWormhole.sol @@ -223,6 +223,13 @@ contract MockWormhole is IWormhole { revert("unsupported verifySignatures in wormhole mock"); } + function verifyCurrentQuorum( + bytes32, /*hash*/ + Signature[] memory /*signatures*/ + ) external pure returns (bool /*valid*/, string memory /*reason*/) { + revert("unsupported verifyCurrentQuorum in wormhole mock"); + } + function parseContractUpgrade(bytes memory /*encodedUpgrade*/ ) external pure From dad07033efac5feab340fb38ef6b43ded96b118e Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Wed, 8 May 2024 08:43:32 -0700 Subject: [PATCH 3/6] Fix imports and get tests to run again --- ethereum/Makefile | 7 +++++-- ethereum/contracts/Messages.sol | 4 +++- ethereum/foundry.toml | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/ethereum/Makefile b/ethereum/Makefile index b199ad3b6a..b591cbc9be 100644 --- a/ethereum/Makefile +++ b/ethereum/Makefile @@ -32,7 +32,7 @@ node_modules: package-lock.json # When adding a new dependency, make sure to specify the exact commit hash, and # the --no-git and --no-commit flags (see lib/forge-std below) .PHONY: forge_dependencies -forge_dependencies: lib/forge-std lib/openzeppelin-contracts +forge_dependencies: lib/forge-std lib/openzeppelin-contracts lib/wormhole-solidity-sdk lib/forge-std: forge install foundry-rs/forge-std@v1.6.1 --no-git --no-commit @@ -40,6 +40,9 @@ lib/forge-std: lib/openzeppelin-contracts: forge install openzeppelin/openzeppelin-contracts@0457042d93d9dfd760dbaa06a4d2f1216fdbe297 --no-git --no-commit +lib/wormhole-solidity-sdk: + forge install wormhole-foundation/wormhole-solidity-sdk@b9e129e65d34827d92fceeed8c87d3ecdfc801d0 --no-git --no-commit + dependencies: node_modules forge_dependencies build: forge_dependencies node_modules ${SOURCE_FILES} @@ -87,4 +90,4 @@ test-push0: dependencies @if grep -qr --include \*.json PUSH0 ./build-forge; then echo "Contract uses PUSH0 instruction" 1>&2; exit 1; fi clean: - rm -rf ganache.log .env node_modules build flattened build-forge ethers-contracts lib/forge-std lib/openzeppelin-contracts + rm -rf ganache.log .env node_modules build flattened build-forge ethers-contracts lib/forge-std lib/openzeppelin-contracts lib/wormhole-solidity-sdk diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index b40008b61e..0813712254 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -6,10 +6,12 @@ pragma experimental ABIEncoderV2; import "./Getters.sol"; import "./Structs.sol"; -import "./libraries/relayer/BytesParsing.sol"; +import "./libraries/external/BytesLib.sol"; +import "wormhole-solidity-sdk/libraries/BytesParsing.sol"; contract Messages is Getters { + using BytesLib for bytes; using BytesParsing for bytes; uint8 private constant ADDRESS_SIZE = 20; // in bytes diff --git a/ethereum/foundry.toml b/ethereum/foundry.toml index 1c4c20e225..8c4bcb7e9b 100644 --- a/ethereum/foundry.toml +++ b/ethereum/foundry.toml @@ -1,5 +1,5 @@ [profile.default] -solc_version = "0.8.4" +solc_version = "0.8.13" optimizer = true optimizer_runs = 200 via_ir = false From f1e3f9ceb9a20155eca63bec5db5ff8258179991 Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Wed, 8 May 2024 09:43:08 -0700 Subject: [PATCH 4/6] Slight gas optimisation --- ethereum/contracts/Messages.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 0813712254..3213b55aa5 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -195,6 +195,7 @@ contract Messages is Getters { function verifyCurrentQuorum(bytes32 hash, Structs.Signature[] memory signatures) public view returns (bool valid, string memory response) { uint32 gsi = getCurrentGuardianSetIndex(); Structs.GuardianSet memory guardianSet = getGuardianSet(gsi); + uint256 guardianCount = guardianSet.keys.length; /** * @dev Checks whether the guardianSet has zero keys @@ -203,7 +204,7 @@ contract Messages is Getters { * key length is 0 and vm.signatures length is 0, this could compromise the integrity of both vm and * signature verification. */ - if(guardianSet.keys.length == 0){ + if(guardianCount == 0){ return (false, "invalid guardian set"); } @@ -213,7 +214,7 @@ contract Messages is Getters { * if making any changes to this, obtain additional peer review. If guardianSet key length is 0 and * vm.signatures length is 0, this could compromise the integrity of both vm and signature verification. */ - if (signatures.length < quorum(guardianSet.keys.length)){ + if (signatures.length < quorum(guardianCount)){ return (false, "no quorum"); } From 50289c0b49ad6769d4a6eb356d39141402853acc Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Wed, 8 May 2024 10:21:11 -0700 Subject: [PATCH 5/6] Remove unused named returns --- ethereum/contracts/Messages.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 3213b55aa5..a4f2465f42 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -192,7 +192,7 @@ contract Messages is Getters { /** * @dev verifyCurrentQuorum serves to validate arbitrary signatures and check quorum against the current guardianSet */ - function verifyCurrentQuorum(bytes32 hash, Structs.Signature[] memory signatures) public view returns (bool valid, string memory response) { + function verifyCurrentQuorum(bytes32 hash, Structs.Signature[] memory signatures) public view returns (bool, string memory) { uint32 gsi = getCurrentGuardianSetIndex(); Structs.GuardianSet memory guardianSet = getGuardianSet(gsi); uint256 guardianCount = guardianSet.keys.length; From 0c828946a210f76ba089aa28502ee6d88c61f010 Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Wed, 10 Jul 2024 16:39:23 -0700 Subject: [PATCH 6/6] Revert back to 0.8.4 + PR feedback --- ethereum/Makefile | 2 +- ethereum/contracts/Messages.sol | 12 ++---------- ethereum/foundry.toml | 3 ++- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/ethereum/Makefile b/ethereum/Makefile index b591cbc9be..343d81c277 100644 --- a/ethereum/Makefile +++ b/ethereum/Makefile @@ -41,7 +41,7 @@ lib/openzeppelin-contracts: forge install openzeppelin/openzeppelin-contracts@0457042d93d9dfd760dbaa06a4d2f1216fdbe297 --no-git --no-commit lib/wormhole-solidity-sdk: - forge install wormhole-foundation/wormhole-solidity-sdk@b9e129e65d34827d92fceeed8c87d3ecdfc801d0 --no-git --no-commit + forge install djb15/wormhole-solidity-sdk@78f4b5e1d1ce4c36e43522c9205eb1e92fde7106 --no-git --no-commit dependencies: node_modules forge_dependencies diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index a4f2465f42..458e21eea4 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -6,12 +6,10 @@ pragma experimental ABIEncoderV2; import "./Getters.sol"; import "./Structs.sol"; -import "./libraries/external/BytesLib.sol"; -import "wormhole-solidity-sdk/libraries/BytesParsing.sol"; +import "wormhole-sdk/libraries/BytesParsing.sol"; contract Messages is Getters { - using BytesLib for bytes; using BytesParsing for bytes; uint8 private constant ADDRESS_SIZE = 20; // in bytes @@ -219,13 +217,7 @@ contract Messages is Getters { } /// @dev Verify the proposed vm.signatures against the guardianSet - (bool signaturesValid, string memory invalidReason) = verifySignatures(hash, signatures, guardianSet); - if(!signaturesValid){ - return (false, invalidReason); - } - - /// If we are here, we've validated the VM is a valid multi-sig that matches the current guardianSet. - return (true, ""); + return verifySignatures(hash, signatures, guardianSet); } /** diff --git a/ethereum/foundry.toml b/ethereum/foundry.toml index 8c4bcb7e9b..fa8ef0d140 100644 --- a/ethereum/foundry.toml +++ b/ethereum/foundry.toml @@ -1,5 +1,5 @@ [profile.default] -solc_version = "0.8.13" +solc_version = "0.8.4" optimizer = true optimizer_runs = 200 via_ir = false @@ -17,6 +17,7 @@ remappings = [ 'ds-test/=lib/forge-std/lib/ds-test/src/', 'forge-std/=lib/forge-std/src/', 'truffle/=node_modules/truffle/', + 'wormhole-sdk/=lib/wormhole-solidity-sdk/src/' ] [fmt]