Skip to content
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.

Sign up for GitHub

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

fix issue 256, 257 #300

Merged
merged 2 commits into from
Dec 1, 2021
Merged

fix issue 256, 257 #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abcd790
fix issue 256, 257
webelf101 Nov 23, 2021
87237f6
update mainnet-fork test script
webelf101 Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 33 additions & 39 deletions solidity/contracts/Gravity.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
);
Expand Down Expand Up @@ -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
Expand All @@ -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."
);

Expand Down Expand Up @@ -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

Expand All @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first substantive change in this PR (other than the signature struct refactor). I guess it doesn't hurt but it's also not very useful. This entire require statement could probably go, and probably should. I just never got around to removing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update is related to the issue #257
Should I ignore this issue? @jtremback


// 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"
);

Expand All @@ -252,16 +248,26 @@ 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."
);

zmanian marked this conversation as resolved.
Show resolved Hide resolved
// Check that enough current validators have signed off on the new validator set
bytes32 newCheckpoint =
makeCheckpoint(_newValidators, _newPowers, _newValsetNonce, state_gravityId);

checkValidatorSignatures(
_currentValidators,
_currentPowers,
_v,
_r,
_s,
_sigs,
newCheckpoint,
state_powerThreshold
);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"
);

Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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"
);

Expand Down Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions solidity/contracts/ReentrantERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,9 +39,7 @@ contract ReentrantERC20 {
addresses,
uint256s,
zero,
new uint8[](0),
bytes32s,
bytes32s,
_sigs,
args
);
}
Expand Down
16 changes: 9 additions & 7 deletions solidity/test-utils/pure.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
56 changes: 26 additions & 30 deletions solidity/test/arbitrary-logic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,50 +163,48 @@ 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(
await getSignerAddresses(validators),
powers,
currentValsetNonce,

sigs.v,
sigs.r,
sigs.s,
sigs,
logicCallArgs
);

Expand Down Expand Up @@ -377,9 +375,7 @@ describe("logicCall Go test hash", function () {
powers,
currentValsetNonce,

sigs.v,
sigs.r,
sigs.s,
sigs,

logicCallArgs
)
Expand Down
4 changes: 1 addition & 3 deletions solidity/test/deployERC20.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,7 @@ async function runTest(opts: {}) {
powers,
currentValsetNonce,

sigs.v,
sigs.r,
sigs.s,
sigs,

txAmounts,
txDestinations,
Expand Down
4 changes: 1 addition & 3 deletions solidity/test/gas-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ describe("Gas tests", function () {
await gravity.testCheckValidatorSignatures(
await getSignerAddresses(validators),
powers,
sigs.v,
sigs.r,
sigs.s,
sigs,
"0x7bc422a00c175cae98cf2f4c36f2f8b63ec51ab8c57fecda9bccf0987ae2d67d",
6666
);
Expand Down
8 changes: 2 additions & 6 deletions solidity/test/happy-path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 2 additions & 6 deletions solidity/test/submitBatch-vs-logicCall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,7 @@ async function runSubmitBatchTest(opts: { batchSize: number }) {
powers,
0,

sigs.v,
sigs.r,
sigs.s,
sigs,

txBatch.amounts,
txBatch.destinations,
Expand Down Expand Up @@ -299,9 +297,7 @@ async function runLogicCallTest(opts: {
powers,
0,

sigs.v,
sigs.r,
sigs.s,
sigs,
logicCallArgs
);

Expand Down
Loading