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

Jump to bottom

use eip 191 standard for presigned transactions #15

Merged
merged 8 commits into from
May 18, 2018
Merged

use eip 191 standard for presigned transactions #15

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a84dea6
implement standard from EIP 191 and fix tests
shine2lay May 17, 2018
e9b8c0f
remove unused code
shine2lay May 17, 2018
4548956
fix changes caused by bad rebase
shine2lay May 17, 2018
835c0e3
Merge remote-tracking branch 'origin/develop' into develop
shine2lay May 18, 2018
0b7d921
merge conflicts and fix eslint and solium errors
shine2lay May 18, 2018
6eb8af8
remove settings not needed
shine2lay May 18, 2018
f7a3212
store msgHash in the Transaction for parameter validation
shine2lay May 18, 2018
f5b4e67
fix eslint
shine2lay May 18, 2018
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
33 changes: 15 additions & 18 deletions contracts/MainChain.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,25 +101,23 @@ contract MainChain is Freezable {
emit Deposit(msg.sender, to, msg.value);
}

/// @dev submit a transaction to be processed
/// @param msgHash sha3 hash of txHash destination value data and VERSION
/// @param txHash transaction hash of the deposit tx in side chain
/// @param destination destination provided in deposit tx in side chain
/// @param value msg.value of deposit tx in side chain
/// @param data data of deposit tx in side chain
/// @param v list of v part of sig
/// @param r list of r part of sig
/// @param s list of s part of sig
function submitTransaction(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s)
notNull(destination)
transactionDoesNotExists(txHash)
txNotBlackListed(txHash)
onlyWhenNotFrozen
public {
/// @dev submit a transaction to be processed
/// @param txHash transaction hash of the deposit tx in side chain
/// @param destination destination provided in deposit tx in side chain
/// @param value msg.value of deposit tx in side chain
/// @param data data of deposit tx in side chain
/// @param v list of v part of sig
/// @param r list of r part of sig
/// @param s list of s part of sig
function submitTransaction(bytes32 txHash, address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s)
notNull(destination)
transactionDoesNotExists(txHash)
txNotBlackListed(txHash)
onlyWhenNotFrozen
public {
require(v.length >= required);

bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION);
require(hashedTxParams == msgHash);
bytes32 msgHash = keccak256(byte(0x19), VERSION, address(this),txHash, destination, value, data);

// execute the transaction after all checking the signatures
require (hasEnoughRequiredSignatures(msgHash, v, r, s));
Expand Down Expand Up @@ -160,7 +158,6 @@ contract MainChain is Freezable {
/// @param destination target destination
/// @param value ether value
/// @param data data payload
// @TODO still need to write tests for emergency withdrawal
function emergencyWithdrawal(address destination, uint256 value, bytes data)
onlyWhenFrozen
ownerExists(msg.sender)
Expand Down
69 changes: 33 additions & 36 deletions contracts/SideChain.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ contract SideChain is Freezable {
// Structs
////////////////////////
struct Transaction {
bytes32 msgHash;
address destination;
uint256 value;
bytes data;
Expand Down Expand Up @@ -76,30 +77,26 @@ contract SideChain is Freezable {
emit Deposit(msg.sender, to, msg.value);
}

/// @dev submit transaction to be processed pending the approvals
/// @param msgHash sha3 hash of txHash destination value data and VERSION
/// @param txHash transaction hash of the deposit tx in main chain
/// @param destination destination provided in deposit tx in main chain
/// @param value msg.value of deposit tx in main chain
/// @param data data of deposit tx in main chain
/// @param v part of sig
/// @param r part of sig
/// @param s part of sig
function submitTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s)
/// @dev submit transaction to be processed pending the approvals
/// @param txHash transaction hash of the deposit tx in main chain
/// @param destination destination provided in deposit tx in main chain
/// @param value msg.value of deposit tx in main chain
/// @param data data of deposit tx in main chain
function submitTransactionSC(bytes32 txHash, address destination, uint256 value, bytes data)
ownerExists(msg.sender)
notNull(destination)
public {
require(!isSignedSC[txHash][msg.sender]);
address signer = ecrecover(msgHash, v, r, s);
require(msg.sender == signer);
bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION);
require(hashedTxParams == msgHash);

bytes32 msgHash = keccak256(txHash, destination, value, data, VERSION);

if (sideChainTx[txHash].destination == address(0)) {
addTransactionSC(txHash, destination, value, data);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we want to remove these lines? Another user could sign the tx with different parameters than ones have been used in sideChainTx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are not necessary because if sideChainTx[txHash].destination != destination then hashedTxParams would be different then msgHash, then line 96 will fail

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this a valid test case? There are two owners. Owner1 submitTransaction with value 1000, then the txHash has been added to sideChainTx. Then owner2 tried to submitTransaction with value 10000, it generates a new msgHash, and it will pass the check require(hashedTxParams == msgHash); We need the require(sideChainTx[txHash].value == value); to revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i am thinking of a way to solve this, just having require(sideChainTx[txHash].value == value) is not enough because we can't do the same for data
I am thinking about either using msgHash as the mapping instead of txhash
or just storing msgHash as a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets just store msgHash, since this is on the sidechain side, gas shouldn't be an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.... i think there is a DOS situation with only having submitTransactionSC, imagine a scenario where an hacker got one of the owner's account, they can submit bogus transaction with proper txHash, in that case then no of the other owners can do anything to fix that situation

require(sideChainTx[txHash].destination == destination);
require(sideChainTx[txHash].value == value);
addTransactionSC(msgHash, txHash, destination, value, data);
}

// this validates that all the parameters are the same as the previous submitter
require(sideChainTx[txHash].msgHash == msgHash);

isSignedSC[txHash][msg.sender] = true;
emit Confirmation(msg.sender, txHash);
executeTransaction(txHash);
Expand Down Expand Up @@ -142,7 +139,7 @@ contract SideChain is Freezable {
public
returns(bool) {
uint count = 0;
for (uint i=0; i<owners.length; i++) {
for (uint i = 0; i<owners.length; i++) {
if (isSignedSC[txHash][owners[i]])
count += 1;
if (count == required)
Expand All @@ -156,7 +153,7 @@ contract SideChain is Freezable {
view
public
returns(uint count) {
for (uint i=0; i<owners.length; i++) {
for (uint i = 0; i<owners.length; i++) {
if (isSignedSC[txHash][owners[i]]) {
count += 1;
}
Expand All @@ -172,13 +169,13 @@ contract SideChain is Freezable {
address[] memory isSignedSCTemp = new address[](owners.length);
uint count = 0;
uint i;
for (i=0; i<owners.length; i++)
for (i = 0; i<owners.length; i++)
if (isSignedSC[txHash][owners[i]]) {
isSignedSCTemp[count] = owners[i];
count += 1;
}
_isSignedSC = new address[](count);
for (i=0; i<count; i++)
for (i = 0; i<count; i++)
_isSignedSC[i] = isSignedSCTemp[i];
}

Expand All @@ -188,14 +185,16 @@ contract SideChain is Freezable {


/// @dev Adds a new transaction to the transaction mapping, if transaction does not exist yet.
/// @param msgHash keccek256 hash of the parameters
/// @param txHash Transaction transactionHash
/// @param destination Transaction target address.
/// @param value Transaction ether value.
/// @param data Transaction data payload.
function addTransactionSC(bytes32 txHash, address destination, uint value, bytes data)
function addTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint value, bytes data)
notNull(destination)
private {
sideChainTx[txHash] = Transaction({
msgHash: msgHash,
destination: destination,
value: value,
executed: false,
Expand Down Expand Up @@ -230,27 +229,25 @@ contract SideChain is Freezable {
// SUBMIT SIGNATURES TO WITHDRAW ON MAINCHAIN
//////////////////////////////////////////////

/// @dev store signature to be submitted in mainchain
/// @param msgHash sha3 hash of txHash destination value data and VERSION
/// @param txHash transaction hash of the deposit tx in side chain
/// @param destination destination provided in deposit tx in side chain
/// @param value msg.value of deposit tx in side chain
/// @param data data of deposit tx in side chain
/// @param v part of sig
/// @param r part of sig
/// @param s part of sig
function submitSignatureMC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s)
/// @dev store signature to be submitted in mainchain
/// @param txHash transaction hash of the deposit tx in side chain
/// @param destination destination provided in deposit tx in side chain
/// @param value msg.value of deposit tx in side chain
/// @param data data of deposit tx in side chain
/// @param v part of sig
/// @param r part of sig
/// @param s part of sig
function submitSignatureMC(bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s)
ownerExists(msg.sender)
notNull(destination)
public {
require(!isSignedMC[txHash][msg.sender]);

bytes32 msgHash = keccak256(byte(0x19), VERSION, address(this),txHash, destination, value, data);

address signer = ecrecover(msgHash, v, r, s);
require(msg.sender == signer);

bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION);
require(hashedTxParams == msgHash);

if(mainChainTxs[txHash].destination == address(0)){
addTransactionMC(txHash, destination, value, data);
}
Expand Down
8 changes: 4 additions & 4 deletions contracts/libs/Freezable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ contract Freezable is MultiSigOwnable {
if (frozenAt == 0 || approvalCount == 0) return false;
else {
uint256 frozenDuration = minTimeFrozen * (frozenDurationRatio ** (approvalCount - 1));
if (now < (frozenAt + frozenDuration)) return true;
if (block.timestamp < (frozenAt + frozenDuration)) return true;
}
return false;
}
Expand Down Expand Up @@ -85,8 +85,8 @@ contract Freezable is MultiSigOwnable {
approvalCount++;

if (frozenAt == 0) {
frozenAt = now;
emit contractFrozen(now);
frozenAt = block.timestamp;
emit contractFrozen(block.timestamp);
}
}

Expand All @@ -100,7 +100,7 @@ contract Freezable is MultiSigOwnable {

if (approvalCount == 0) {
frozenAt = 0;
emit contractUnFrozen(now);
emit contractUnFrozen(block.timestamp);
}
}
}
13 changes: 6 additions & 7 deletions contracts/libs/MultiSigOwnable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,10 @@ contract MultiSigOwnable {
}

modifier validRequirement(uint ownerCount, uint8 _required) {
require(
ownerCount <= MAX_OWNER_COUNT
&& _required <= ownerCount
&& _required != 0
&& ownerCount != 0);
require(ownerCount <= MAX_OWNER_COUNT);
require(_required <= ownerCount);
require(_required != 0);
require(ownerCount != 0);
_;
}

Expand Down Expand Up @@ -84,9 +83,9 @@ contract MultiSigOwnable {
ownerExists(owner)
public {
isOwner[owner] = false;
for (uint i=0; i< owners.length - 1; i++) {
for (uint i = 0; i < owners.length - 1; i++) {
if (owners[i] == owner) {
owners[i] = owners[ owners.length - 1];
owners[i] = owners[owners.length - 1];
break;
}
}
Expand Down
10 changes: 6 additions & 4 deletions test/MainChainUnitTest/addBlackListUnitTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ let txHash;
let toAddress;
let value;
let version;
let contractAddress;

contract('MainChain: addBlackList Unit Test', function(accounts) {
beforeEach(async function() {
Expand All @@ -27,6 +28,7 @@ contract('MainChain: addBlackList Unit Test', function(accounts) {
toAddress = mainchainInstance.address;
value = 0;
version = await mainchainInstance.VERSION.call();
contractAddress = mainchainInstance.address;
});

it('checks that addBlackList work as intended if all the condition are valid', async function() {
Expand All @@ -44,14 +46,14 @@ contract('MainChain: addBlackList Unit Test', function(accounts) {
);
const sigs = utils.multipleSignedTransaction(
[0, 1],
contractAddress,
txHash,
toAddress,
value,
addBlackListData,
version,
);
await mainchainInstance.submitTransaction(
sigs.msgHash,
txHash,
toAddress,
value,
Expand Down Expand Up @@ -90,14 +92,14 @@ contract('MainChain: addBlackList Unit Test', function(accounts) {
);
let sigs = utils.multipleSignedTransaction(
[0, 1],
contractAddress,
txHash,
toAddress,
value,
addBlackListData,
version,
);
await mainchainInstance.submitTransaction(
sigs.msgHash,
txHash,
toAddress,
value,
Expand All @@ -114,14 +116,14 @@ contract('MainChain: addBlackList Unit Test', function(accounts) {
// const data = mainchainInstance.contract.addBlackList.getData(txHash);
sigs = utils.multipleSignedTransaction(
[0, 1],
contractAddress,
txHashes[1],
toAddress,
value,
addBlackListData,
version,
);
const res = await mainchainInstance.submitTransaction(
sigs.msgHash,
txHashes[1],
toAddress,
value,
Expand All @@ -141,14 +143,14 @@ contract('MainChain: addBlackList Unit Test', function(accounts) {
);
const sigs = utils.multipleSignedTransaction(
[0, 1],
contractAddress,
txHash,
toAddress,
value,
addBlackListData,
version,
);
const res = await mainchainInstance.submitTransaction(
sigs.msgHash,
txHash,
toAddress,
value,
Expand Down
Loading