certusone
/
wormhole
mirror of https://github.com/certusone/wormhole.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

add: safety check against signatory being address(0) (#2021) 

* add: safety check against signatory being address(0)

Change-Id: I0e0b32c9d9b8501cfe57b8c672441314774c009f

* evm: add revert message and fix tests

Co-authored-by: Rasul <rasul.yunusov282750@gmail.com>
Co-authored-by: A5 Pickle <a5-pickle@users.noreply.github.com>
This commit is contained in:
Hendrik Hofstadt 2023-01-11 18:32:48 +01:00 committed by GitHub
parent 1e6aa2b48e
commit 9b50158a3f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 102 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
ethereum/contracts/Messages.sol
View File

@ -76,6 +76,10 @@ contract Messages is Getters {
uint256 guardianCount = guardianSet.keys.length;
for (uint i = 0; i < signatures.length; i++) {
Structs.Signature memory sig = signatures[i];
address signatory = ecrecover(hash, sig.v, sig.r, sig.s);
// ecrecover returns 0 for invalid signatures. We explicitly require valid signatures to avoid unexpected
// behaviour due to the default storage slot value also being 0.
require(signatory != address(0), "ecrecover failed with signature");
/// Ensure that provided signature indices are ascending only
require(i == 0 || sig.guardianIndex > lastIndex, "signature indices must be ascending");
@ -90,7 +94,7 @@ contract Messages is Getters {
require(sig.guardianIndex < guardianCount, "guardian index out of bounds");
/// Check to see if the signer of the signature does not match a specific Guardian key at the provided index
if(ecrecover(hash, sig.v, sig.r, sig.s) != guardianSet.keys[sig.guardianIndex]){
if(signatory != guardianSet.keys[sig.guardianIndex]){
return (false, "VM signature invalid");
}
}

146
ethereum/forge-test/Messages.t.sol
View File

@ -7,65 +7,113 @@ import "../contracts/Messages.sol";
import "../contracts/Structs.sol";
import "forge-std/Test.sol";
contract TestMessages is Messages, Test {
address constant testGuardianPub = 0xbeFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe;
contract TestMessages is Test {
uint256 constant testGuardian = 93941733246223705020089879371323733820373732307041878556247502674739205313440;
function testQuorum() public {
assertEq(quorum(0), 1);
assertEq(quorum(1), 1);
assertEq(quorum(2), 2);
assertEq(quorum(3), 3);
assertEq(quorum(4), 3);
assertEq(quorum(5), 4);
assertEq(quorum(6), 5);
assertEq(quorum(7), 5);
assertEq(quorum(8), 6);
assertEq(quorum(9), 7);
assertEq(quorum(10), 7);
assertEq(quorum(11), 8);
assertEq(quorum(12), 9);
assertEq(quorum(19), 13);
assertEq(quorum(20), 14);
Messages messages;
Structs.GuardianSet guardianSet;
function setUp() public {
messages = new Messages();
// initialize guardian set with one guardian
address[] memory keys = new address[](1);
keys[0] = vm.addr(testGuardian);
guardianSet = Structs.GuardianSet(keys, 0);
require(messages.quorum(guardianSet.keys.length) == 1, "Quorum should be 1");
}
function testQuorumCanAlwaysBeReached(uint numGuardians) public {
if (numGuardians == 0) {
return;
}
function testQuorum() public {
assertEq(messages.quorum(0), 1);
assertEq(messages.quorum(1), 1);
assertEq(messages.quorum(2), 2);
assertEq(messages.quorum(3), 3);
assertEq(messages.quorum(4), 3);
assertEq(messages.quorum(5), 4);
assertEq(messages.quorum(6), 5);
assertEq(messages.quorum(7), 5);
assertEq(messages.quorum(8), 6);
assertEq(messages.quorum(9), 7);
assertEq(messages.quorum(10), 7);
assertEq(messages.quorum(11), 8);
assertEq(messages.quorum(12), 9);
assertEq(messages.quorum(19), 13);
assertEq(messages.quorum(20), 14);
}
function testQuorumCanAlwaysBeReached(uint256 numGuardians) public {
vm.assume(numGuardians > 0);
if (numGuardians >= 256) {
vm.expectRevert("too many guardians");
}
// test that quorums is never greater than the number of guardians
assert(quorum(numGuardians) <= numGuardians);
assert(messages.quorum(numGuardians) <= numGuardians);
}
// This test ensures that submitting invalid signatures for non-existent
// guardians fails.
//
// The main purpose of this test is to ensure that there's no surprising
// behaviour arising from solidity's handling of invalid signatures and out of
// bounds memory access. In particular, pubkey recovery of an invalid
// signature returns 0, and in some cases out of bounds memory access also
// just returns 0.
function testOutOfBoundsSignature() public {
// Initialise a guardian set with a single guardian.
address[] memory keys = new address[](1);
keys[0] = testGuardianPub;
Structs.GuardianSet memory guardianSet = Structs.GuardianSet(keys, 0);
require(quorum(guardianSet.keys.length) == 1, "Quorum should be 1");
// This test ensures that submitting more signatures than expected will
// trigger a "guardian index out of bounds" error.
function testCannotVerifySignaturesWithOutOfBoundsSignature(bytes memory encoded) public {
vm.assume(encoded.length > 0);
// Two invalid signatures, for guardian index 2 and 3 respectively.
// These guardian indices are out of bounds for the guardian set.
bytes32 message = "hello";
Structs.Signature memory bad1 = Structs.Signature(message, 0, 0, 2);
Structs.Signature memory bad2 = Structs.Signature(message, 0, 0, 3);
// ecrecover on an invalid signature returns 0 instead of reverting
require(ecrecover(message, bad1.v, bad1.r, bad1.s) == address(0), "ecrecover should return the 0 address for an invalid signature");
bytes32 message = keccak256(encoded);
Structs.Signature[] memory badSigs = new Structs.Signature[](2);
badSigs[0] = bad1;
badSigs[1] = bad2;
vm.expectRevert(bytes("guardian index out of bounds"));
verifySignatures(0, badSigs, guardianSet);
// First generate a legitimate signature.
Structs.Signature memory goodSignature = Structs.Signature(message, 0, 0, 0);
(goodSignature.v, goodSignature.r, goodSignature.s) = vm.sign(testGuardian, message);
assertEq(ecrecover(message, goodSignature.v, goodSignature.r, goodSignature.s), vm.addr(testGuardian));
// Reuse legitimate signature above for the next signature. This will
// bypass the "invalid signature" revert.
Structs.Signature memory outOfBoundsSignature = goodSignature;
outOfBoundsSignature.guardianIndex = 1;
// Attempt to verify signatures.
Structs.Signature[] memory sigs = new Structs.Signature[](2);
sigs[0] = goodSignature;
sigs[1] = outOfBoundsSignature;
vm.expectRevert("guardian index out of bounds");
messages.verifySignatures(message, sigs, guardianSet);
}
// This test ensures that submitting an invalid signature fails when
// verifySignatures is called. Calling ecrecover should fail.
function testCannotVerifySignaturesWithInvalidSignature(bytes memory encoded) public {
vm.assume(encoded.length > 0);
bytes32 message = keccak256(encoded);
// Generate an invalid signature.
Structs.Signature memory badSignature = Structs.Signature(message, 0, 0, 0);
assertEq(ecrecover(message, badSignature.v, badSignature.r, badSignature.s), address(0));
// Attempt to verify signatures.
Structs.Signature[] memory sigs = new Structs.Signature[](2);
sigs[0] = badSignature;
vm.expectRevert("ecrecover failed with signature");
messages.verifySignatures(message, sigs, guardianSet);
}
function testVerifySignatures(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.verifySignatures(message, sigs, guardianSet);
assertEq(valid, true);
assertEq(bytes(reason).length, 0);
}
}