From abcd7909af5d0f126b9b7439df1343782029a8bc Mon Sep 17 00:00:00 2001 From: webelf101 Date: Tue, 23 Nov 2021 21:36:36 +0800 Subject: [PATCH 1/2] fix issue 256, 257 --- solidity/contracts/Gravity.sol | 72 +++++++++++------------ solidity/contracts/ReentrantERC20.sol | 6 +- solidity/test-utils/pure.ts | 16 ++--- solidity/test/arbitrary-logic.ts | 56 ++++++++---------- solidity/test/deployERC20.ts | 4 +- solidity/test/gas-test.ts | 4 +- solidity/test/happy-path.ts | 8 +-- solidity/test/submitBatch-vs-logicCall.ts | 8 +-- solidity/test/submitBatch.ts | 56 ++++++++---------- solidity/test/updateValset.ts | 36 ++++++------ 10 files changed, 119 insertions(+), 147 deletions(-) diff --git a/solidity/contracts/Gravity.sol b/solidity/contracts/Gravity.sol index 15bca4aab..b8f8ad1db 100644 --- a/solidity/contracts/Gravity.sol +++ b/solidity/contracts/Gravity.sol @@ -26,6 +26,12 @@ struct LogicCallArgs { uint256 invalidationNonce; } +struct Signature { + uint8 v; + bytes32 r; + bytes32 s; +} + contract Gravity is ReentrancyGuard { using SafeMath for uint256; using SafeERC20 for IERC20; @@ -97,18 +103,14 @@ contract Gravity is ReentrancyGuard { function testCheckValidatorSignatures( address[] memory _currentValidators, uint256[] memory _currentPowers, - uint8[] memory _v, - bytes32[] memory _r, - bytes32[] memory _s, + Signature[] memory _sigs, bytes32 _theHash, uint256 _powerThreshold ) public pure { checkValidatorSignatures( _currentValidators, _currentPowers, - _v, - _r, - _s, + _sigs, _theHash, _powerThreshold ); @@ -165,9 +167,7 @@ contract Gravity is ReentrancyGuard { address[] memory _currentValidators, uint256[] memory _currentPowers, // The current validator's signatures - uint8[] memory _v, - bytes32[] memory _r, - bytes32[] memory _s, + Signature[] memory _sigs, // This is what we are checking they have signed bytes32 _theHash, uint256 _powerThreshold @@ -177,10 +177,10 @@ contract Gravity is ReentrancyGuard { for (uint256 i = 0; i < _currentValidators.length; i++) { // If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation // (In a valid signature, it is either 27 or 28) - if (_v[i] != 0) { + if (_sigs[i].v != 0) { // Check that the current validator has signed off on the hash require( - verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]), + verifySig(_currentValidators[i], _theHash, _sigs[i].v, _sigs[i].r, _sigs[i].s), "Validator signature does not match." ); @@ -217,9 +217,7 @@ contract Gravity is ReentrancyGuard { uint256[] memory _currentPowers, uint256 _currentValsetNonce, // These are arrays of the parts of the current validator's signatures - uint8[] memory _v, - bytes32[] memory _r, - bytes32[] memory _s + Signature[] memory _sigs ) public nonReentrant { // CHECKS @@ -230,14 +228,12 @@ contract Gravity is ReentrancyGuard { ); // Check that new validators and powers set is well-formed - require(_newValidators.length == _newPowers.length, "Malformed new validator set"); + require(_newValidators.length == _newPowers.length || _newValidators.length == 0, "Malformed new validator set"); // Check that current validators, powers, and signatures (v,r,s) set is well-formed require( _currentValidators.length == _currentPowers.length && - _currentValidators.length == _v.length && - _currentValidators.length == _r.length && - _currentValidators.length == _s.length, + _currentValidators.length == _sigs.length, "Malformed current validator set" ); @@ -252,6 +248,18 @@ contract Gravity is ReentrancyGuard { "Supplied current validators and powers do not match checkpoint." ); + uint256 cumulativePower = 0; + for (uint256 i = 0; i < _newPowers.length; i++) { + cumulativePower = cumulativePower + _newPowers[i]; + if (cumulativePower > state_powerThreshold) { + break; + } + } + require( + cumulativePower > state_powerThreshold, + "Submitted validator set signatures do not have enough power." + ); + // Check that enough current validators have signed off on the new validator set bytes32 newCheckpoint = makeCheckpoint(_newValidators, _newPowers, _newValsetNonce, state_gravityId); @@ -259,9 +267,7 @@ contract Gravity is ReentrancyGuard { checkValidatorSignatures( _currentValidators, _currentPowers, - _v, - _r, - _s, + _sigs, newCheckpoint, state_powerThreshold ); @@ -290,9 +296,7 @@ contract Gravity is ReentrancyGuard { uint256[] memory _currentPowers, uint256 _currentValsetNonce, // These are arrays of the parts of the validators signatures - uint8[] memory _v, - bytes32[] memory _r, - bytes32[] memory _s, + Signature[] memory _sigs, // The batch of transactions uint256[] memory _amounts, address[] memory _destinations, @@ -320,9 +324,7 @@ contract Gravity is ReentrancyGuard { // Check that current validators, powers, and signatures (v,r,s) set is well-formed require( _currentValidators.length == _currentPowers.length && - _currentValidators.length == _v.length && - _currentValidators.length == _r.length && - _currentValidators.length == _s.length, + _currentValidators.length == _sigs.length, "Malformed current validator set" ); @@ -347,9 +349,7 @@ contract Gravity is ReentrancyGuard { checkValidatorSignatures( _currentValidators, _currentPowers, - _v, - _r, - _s, + _sigs, // Get hash of the transaction batch and checkpoint keccak256( abi.encode( @@ -407,9 +407,7 @@ contract Gravity is ReentrancyGuard { uint256[] memory _currentPowers, uint256 _currentValsetNonce, // These are arrays of the parts of the validators signatures - uint8[] memory _v, - bytes32[] memory _r, - bytes32[] memory _s, + Signature[] memory _sigs, LogicCallArgs memory _args ) public nonReentrant { // CHECKS scoped to reduce stack depth @@ -426,9 +424,7 @@ contract Gravity is ReentrancyGuard { // Check that current validators, powers, and signatures (v,r,s) set is well-formed require( _currentValidators.length == _currentPowers.length && - _currentValidators.length == _v.length && - _currentValidators.length == _r.length && - _currentValidators.length == _s.length, + _currentValidators.length == _sigs.length, "Malformed current validator set" ); @@ -479,9 +475,7 @@ contract Gravity is ReentrancyGuard { checkValidatorSignatures( _currentValidators, _currentPowers, - _v, - _r, - _s, + _sigs, // Get hash of the transaction batch and checkpoint argsHash, state_powerThreshold diff --git a/solidity/contracts/ReentrantERC20.sol b/solidity/contracts/ReentrantERC20.sol index 1483ed2c1..2f6930172 100644 --- a/solidity/contracts/ReentrantERC20.sol +++ b/solidity/contracts/ReentrantERC20.sol @@ -15,8 +15,8 @@ contract ReentrantERC20 { function transfer(address recipient, uint256 amount) public returns (bool) { // _currentValidators, _currentPowers, _currentValsetNonce, _v, _r, _s, _args);( address[] memory addresses = new address[](0); - bytes32[] memory bytes32s = new bytes32[](0); uint256[] memory uint256s = new uint256[](0); + Signature[] memory _sigs = new Signature[](0); bytes memory bytess = new bytes(0); uint256 zero = 0; LogicCallArgs memory args; @@ -39,9 +39,7 @@ contract ReentrantERC20 { addresses, uint256s, zero, - new uint8[](0), - bytes32s, - bytes32s, + _sigs, args ); } diff --git a/solidity/test-utils/pure.ts b/solidity/test-utils/pure.ts index abafdfbbd..8bac3f1bf 100644 --- a/solidity/test-utils/pure.ts +++ b/solidity/test-utils/pure.ts @@ -25,22 +25,24 @@ export function makeCheckpoint( return checkpoint; } +export type Sig = { + v: number, + r: string, + s: string +} + export async function signHash(signers: Signer[], hash: string) { - let v: number[] = []; - let r: string[] = []; - let s: string[] = []; + let sigs: Sig[] = []; for (let i = 0; i < signers.length; i = i + 1) { const sig = await signers[i].signMessage(ethers.utils.arrayify(hash)); const address = await signers[i].getAddress(); const splitSig = ethers.utils.splitSignature(sig); - v.push(splitSig.v!); - r.push(splitSig.r); - s.push(splitSig.s); + sigs.push({ v: splitSig.v!, r: splitSig.r, s: splitSig.s }); } - return { v, r, s }; + return sigs; } export function makeTxBatchHash( diff --git a/solidity/test/arbitrary-logic.ts b/solidity/test/arbitrary-logic.ts index 4d44a1062..1c58ef46f 100644 --- a/solidity/test/arbitrary-logic.ts +++ b/solidity/test/arbitrary-logic.ts @@ -163,40 +163,40 @@ async function runTest(opts: { } if (opts.badValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; } if (opts.zeroedValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; // Then zero it out to skip evaluation - sigs.v[1] = 0; + sigs[1].v = 0; } if (opts.notEnoughPower) { // zero out enough signatures that we dip below the threshold - sigs.v[1] = 0; - sigs.v[2] = 0; - sigs.v[3] = 0; - sigs.v[5] = 0; - sigs.v[6] = 0; - sigs.v[7] = 0; - sigs.v[9] = 0; - sigs.v[11] = 0; - sigs.v[13] = 0; + sigs[1].v = 0; + sigs[2].v = 0; + sigs[3].v = 0; + sigs[5].v = 0; + sigs[6].v = 0; + sigs[7].v = 0; + sigs[9].v = 0; + sigs[11].v = 0; + sigs[13].v = 0; } if (opts.barelyEnoughPower) { // Stay just above the threshold - sigs.v[1] = 0; - sigs.v[2] = 0; - sigs.v[3] = 0; - sigs.v[5] = 0; - sigs.v[6] = 0; - sigs.v[7] = 0; - sigs.v[9] = 0; - sigs.v[11] = 0; + sigs[1].v = 0; + sigs[2].v = 0; + sigs[3].v = 0; + sigs[5].v = 0; + sigs[6].v = 0; + sigs[7].v = 0; + sigs[9].v = 0; + sigs[11].v = 0; } await gravity.submitLogicCall( @@ -204,9 +204,7 @@ async function runTest(opts: { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, logicCallArgs ); @@ -377,9 +375,7 @@ describe("logicCall Go test hash", function () { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, logicCallArgs ) diff --git a/solidity/test/deployERC20.ts b/solidity/test/deployERC20.ts index d28b0de6d..95da9b14f 100644 --- a/solidity/test/deployERC20.ts +++ b/solidity/test/deployERC20.ts @@ -137,9 +137,7 @@ async function runTest(opts: {}) { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, txAmounts, txDestinations, diff --git a/solidity/test/gas-test.ts b/solidity/test/gas-test.ts index f2b346894..432e3333a 100644 --- a/solidity/test/gas-test.ts +++ b/solidity/test/gas-test.ts @@ -61,9 +61,7 @@ describe("Gas tests", function () { await gravity.testCheckValidatorSignatures( await getSignerAddresses(validators), powers, - sigs.v, - sigs.r, - sigs.s, + sigs, "0x7bc422a00c175cae98cf2f4c36f2f8b63ec51ab8c57fecda9bccf0987ae2d67d", 6666 ); diff --git a/solidity/test/happy-path.ts b/solidity/test/happy-path.ts index 76e531a96..ee1839e2d 100644 --- a/solidity/test/happy-path.ts +++ b/solidity/test/happy-path.ts @@ -76,9 +76,7 @@ describe("Gravity happy path valset update + batch submit", function () { valset0.powers, valset0.nonce, - sigs1.v, - sigs1.r, - sigs1.s + sigs1 ); expect((await gravity.functions.state_lastValsetCheckpoint())[0]).to.equal(checkpoint1); @@ -150,9 +148,7 @@ describe("Gravity happy path valset update + batch submit", function () { valset1.powers, valset1.nonce, - sigs.v, - sigs.r, - sigs.s, + sigs, txAmounts, txDestinations, diff --git a/solidity/test/submitBatch-vs-logicCall.ts b/solidity/test/submitBatch-vs-logicCall.ts index 0732374d4..1db997e04 100644 --- a/solidity/test/submitBatch-vs-logicCall.ts +++ b/solidity/test/submitBatch-vs-logicCall.ts @@ -157,9 +157,7 @@ async function runSubmitBatchTest(opts: { batchSize: number }) { powers, 0, - sigs.v, - sigs.r, - sigs.s, + sigs, txBatch.amounts, txBatch.destinations, @@ -299,9 +297,7 @@ async function runLogicCallTest(opts: { powers, 0, - sigs.v, - sigs.r, - sigs.s, + sigs, logicCallArgs ); diff --git a/solidity/test/submitBatch.ts b/solidity/test/submitBatch.ts index fef0bbdd1..ec3bb2717 100644 --- a/solidity/test/submitBatch.ts +++ b/solidity/test/submitBatch.ts @@ -116,40 +116,40 @@ async function runTest(opts: { } if (opts.badValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; } if (opts.zeroedValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; // Then zero it out to skip evaluation - sigs.v[1] = 0; + sigs[1].v = 0; } if (opts.notEnoughPower) { // zero out enough signatures that we dip below the threshold - sigs.v[1] = 0; - sigs.v[2] = 0; - sigs.v[3] = 0; - sigs.v[5] = 0; - sigs.v[6] = 0; - sigs.v[7] = 0; - sigs.v[9] = 0; - sigs.v[11] = 0; - sigs.v[13] = 0; + sigs[1].v = 0; + sigs[2].v = 0; + sigs[3].v = 0; + sigs[5].v = 0; + sigs[6].v = 0; + sigs[7].v = 0; + sigs[9].v = 0; + sigs[11].v = 0; + sigs[13].v = 0; } if (opts.barelyEnoughPower) { // Stay just above the threshold - sigs.v[1] = 0; - sigs.v[2] = 0; - sigs.v[3] = 0; - sigs.v[5] = 0; - sigs.v[6] = 0; - sigs.v[7] = 0; - sigs.v[9] = 0; - sigs.v[11] = 0; + sigs[1].v = 0; + sigs[2].v = 0; + sigs[3].v = 0; + sigs[5].v = 0; + sigs[6].v = 0; + sigs[7].v = 0; + sigs[9].v = 0; + sigs[11].v = 0; } await gravity.submitBatch( @@ -157,9 +157,7 @@ async function runTest(opts: { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, txAmounts, txDestinations, @@ -308,9 +306,7 @@ describe("submitBatch Go test hash", function () { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, txAmounts, txDestinations, diff --git a/solidity/test/updateValset.ts b/solidity/test/updateValset.ts index b28acbdc5..57e07a433 100644 --- a/solidity/test/updateValset.ts +++ b/solidity/test/updateValset.ts @@ -67,31 +67,31 @@ async function runTest(opts: { let sigs = await signHash(validators, checkpoint); if (opts.badValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; } if (opts.zeroedValidatorSig) { // Switch the first sig for the second sig to screw things up - sigs.v[1] = sigs.v[0]; - sigs.r[1] = sigs.r[0]; - sigs.s[1] = sigs.s[0]; + sigs[1].v = sigs[0].v; + sigs[1].r = sigs[0].r; + sigs[1].s = sigs[0].s; // Then zero it out to skip evaluation - sigs.v[1] = 0; + sigs[1].v = 0; } if (opts.notEnoughPower) { // zero out enough signatures that we dip below the threshold - sigs.v[1] = 0; - sigs.v[2] = 0; - sigs.v[3] = 0; - sigs.v[5] = 0; - sigs.v[6] = 0; - sigs.v[7] = 0; - sigs.v[9] = 0; - sigs.v[11] = 0; - sigs.v[13] = 0; + sigs[1].v = 0; + sigs[2].v = 0; + sigs[3].v = 0; + sigs[5].v = 0; + sigs[6].v = 0; + sigs[7].v = 0; + sigs[9].v = 0; + sigs[11].v = 0; + sigs[13].v = 0; } if (opts.malformedCurrentValset) { @@ -106,9 +106,7 @@ async function runTest(opts: { await getSignerAddresses(validators), powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s + sigs ); return { gravity, checkpoint }; From 87237f698286995731ca2e3d14da2cd1f85d9d4e Mon Sep 17 00:00:00 2001 From: webelf101 Date: Wed, 24 Nov 2021 00:47:29 +0800 Subject: [PATCH 2/2] update mainnet-fork test script --- solidity/tests_mainnet_fork/uniswap-logic.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/solidity/tests_mainnet_fork/uniswap-logic.ts b/solidity/tests_mainnet_fork/uniswap-logic.ts index e18370f26..7ec06f988 100644 --- a/solidity/tests_mainnet_fork/uniswap-logic.ts +++ b/solidity/tests_mainnet_fork/uniswap-logic.ts @@ -227,9 +227,7 @@ async function runTest() { powers, currentValsetNonce, - sigs.v, - sigs.r, - sigs.s, + sigs, logicCallArgs );