diff --git a/ethereum/Makefile b/ethereum/Makefile index b199ad3b6a..343d81c277 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 djb15/wormhole-solidity-sdk@78f4b5e1d1ce4c36e43522c9205eb1e92fde7106 --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 ec6b43a514..458e21eea4 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -6,7 +6,7 @@ pragma experimental ABIEncoderV2; import "./Getters.sol"; import "./Structs.sol"; -import "./libraries/relayer/BytesParsing.sol"; +import "wormhole-sdk/libraries/BytesParsing.sol"; contract Messages is Getters { @@ -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,39 @@ 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, string memory) { + uint32 gsi = getCurrentGuardianSetIndex(); + Structs.GuardianSet memory guardianSet = getGuardianSet(gsi); + uint256 guardianCount = guardianSet.keys.length; + + /** + * @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(guardianCount == 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(guardianCount)){ + return (false, "no quorum"); + } + + /// @dev Verify the proposed vm.signatures against the guardianSet + return verifySignatures(hash, signatures, guardianSet); + } + /** * @dev parseVM serves to parse an encodedVM into a vm struct * - it intentionally performs no validation functions, it simply parses raw into a struct 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/ethereum/foundry.toml b/ethereum/foundry.toml index 1c4c20e225..fa8ef0d140 100644 --- a/ethereum/foundry.toml +++ b/ethereum/foundry.toml @@ -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] 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