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

Sign messages in a way that real-world wallets expect #205

Merged
merged 4 commits into from
May 17, 2018
Merged

Sign messages in a way that real-world wallets expect #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e30a12
Add message prefix to ERC191 message hash
May 11, 2018
4bb1682
Add support for geth-style personalMessage signing
area May 15, 2018
4afdc90
Add support for Trezor-style message signing
area May 15, 2018
7c6c5b2
Add tests for Trezor-style signatures
area May 16, 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
37 changes: 23 additions & 14 deletions contracts/ColonyTask.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,29 @@ contract ColonyTask is ColonyStorage, DSMath {
return taskChangeNonce;
}

function getSignedMessageHash(uint256 _value, bytes _data, uint8 _mode) private returns (bytes32 txHash) {
bytes32 msgHash = keccak256(
address(this),
address(this),
_value,
_data,
taskChangeNonce
);
if (_mode==0) {
// 'Normal' mode - geth, etc.
return keccak256("\x19Ethereum Signed Message:\n32", msgHash);
} else {
// Trezor mode
// Correct incantation helpfully cribbed from https://github.com/trezor/trezor-mcu/issues/163#issuecomment-368435292
return keccak256("\x19Ethereum Signed Message:\n\x20", msgHash);
}
}

function executeTaskChange(
uint8[] _sigV,
bytes32[] _sigR,
bytes32[] _sigS,
uint8[] _mode,
uint256 _value,
bytes _data) public
{
Expand All @@ -156,32 +175,22 @@ contract ColonyTask is ColonyStorage, DSMath {
(sig, taskId) = deconstructCall(_data);
Task storage task = tasks[taskId];
require(!task.finalized);

uint8[2] storage _reviewers = reviewers[sig];
uint8 r1 = _reviewers[0];
uint8 r2 = _reviewers[1];
// Prevent calls to non registered /arbitrary function on the contract
// Checks at least one of the two reviewers registered is different to the task manager
require(r1 != MANAGER || r2 != MANAGER);

// Follows ERC191 signature scheme: https://github.com/ethereum/EIPs/issues/191
bytes32 txHash = keccak256(
byte(0x19),
byte(0),
address(this),
address(this),
_value,
_data,
taskChangeNonce);

address[] memory reviewerAddresses = new address[](2);
for (uint i = 0; i < 2; i++) {
reviewerAddresses[i] = ecrecover(txHash, _sigV[i], _sigR[i], _sigS[i]);
reviewerAddresses[i] = ecrecover(getSignedMessageHash(_value, _data, _mode[i]), _sigV[i], _sigR[i], _sigS[i]);
}

require(task.roles[r1].user == reviewerAddresses[0] || task.roles[r1].user == reviewerAddresses[1]);
require(task.roles[r2].user == reviewerAddresses[0] || task.roles[r2].user == reviewerAddresses[1]);

taskChangeNonce = taskChangeNonce + 1;
require(address(this).call.value(_value)(_data));
}
Expand All @@ -199,7 +208,7 @@ contract ColonyTask is ColonyStorage, DSMath {

function revealTaskWorkRating(uint256 _id, uint8 _role, uint8 _rating, bytes32 _salt) public
taskWorkRatingRevealOpen(_id)
{
{
// MAYBE: we should hash these the other way around, i.e. generateSecret(_rating, _salt)
bytes32 ratingSecret = generateSecret(_salt, _rating);
require(ratingSecret == taskWorkRatings[_id].secret[_role]);
Expand Down
3 changes: 2 additions & 1 deletion contracts/IColony.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,11 @@ contract IColony {
/// @param _sigV recovery id
/// @param _sigR r output of the ECDSA signature of the transaction
/// @param _sigS s output of the ECDSA signature of the transaction
/// @param _mode How the signature was generated - 0 for Geth-style (usual), 1 for Trezor-style (only Trezor does this)
/// @param _value The transaction value, i.e. number of wei to be sent when the transaction is executed
/// Currently we only accept 0 value transactions but this is kept as a future option
/// @param _data The transaction data
function executeTaskChange(uint8[] _sigV, bytes32[] _sigR, bytes32[] _sigS, uint256 _value, bytes _data) public;
function executeTaskChange(uint8[] _sigV, bytes32[] _sigR, bytes32[] _sigS, uint8[] _mode, uint256 _value, bytes _data) public;

/// @notice Submit a hashed secret of the rating for work in task `_id` which was performed by user with task role id `_role`
/// Allowed within 5 days period starting which whichever is first from either the deliverable being submitted or the dueDate been reached
Expand Down
8 changes: 4 additions & 4 deletions gasCosts/gasCosts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,14 @@ contract("All", accounts => {
// setTaskBrief
txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH);
sigs = await createSignatures(colony, [MANAGER, WORKER], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

// setTaskDueDate
let dueDate = await currentBlockTime();
dueDate += SECONDS_PER_DAY * 5;
txData = await colony.contract.setTaskDueDate.getData(1, dueDate);
sigs = await createSignatures(colony, [MANAGER, WORKER], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

// moveFundsBetweenPots
await colony.moveFundsBetweenPots(1, 2, 150, tokenAddress);
Expand All @@ -134,12 +134,12 @@ contract("All", accounts => {
// setTaskEvaluatorPayout
txData = await colony.contract.setTaskEvaluatorPayout.getData(1, tokenAddress, 40);
sigs = await createSignatures(colony, [MANAGER, EVALUATOR], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

// setTaskWorkerPayout
txData = await colony.contract.setTaskWorkerPayout.getData(1, tokenAddress, 100);
sigs = await createSignatures(colony, [MANAGER, WORKER], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

// submitTaskDeliverable
await colony.submitTaskDeliverable(1, DELIVERABLE_HASH, { from: WORKER, gasPrice });
Expand Down
6 changes: 3 additions & 3 deletions helpers/test-data-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export async function setupAssignedTask({ colonyNetwork, colony, dueDate, domain
const txData = await colony.contract.setTaskDueDate.getData(taskId, dueDateTimestamp);
const signers = [MANAGER, WORKER];
const sigs = await createSignatures(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

return taskId;
}
Expand Down Expand Up @@ -85,11 +85,11 @@ export async function setupFundedTask({

txData = await colony.contract.setTaskEvaluatorPayout.getData(taskId, tokenAddress, evaluatorPayout.toString());
sigs = await createSignatures(colony, [MANAGER, EVALUATOR], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

txData = await colony.contract.setTaskWorkerPayout.getData(taskId, tokenAddress, workerPayout.toString());
sigs = await createSignatures(colony, [MANAGER, WORKER], 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);
return taskId;
}

Expand Down
38 changes: 31 additions & 7 deletions helpers/test-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,22 +203,46 @@ export async function createSignatures(colony, signers, value, data) {
const destinationAddress = colony.address;
const nonce = await colony.getTaskChangeNonce.call();
const accountsJson = JSON.parse(fs.readFileSync("./test-accounts.json", "utf8"));
const input = `${"0x19"}${"00"}${sourceAddress.slice(2)}${destinationAddress.slice(2)}${web3Utils.padLeft(
value.toString("16"),
"64",
"0"
)}${data.slice(2)}${web3Utils.padLeft(nonce.toString("16"), "64", "0")}`; // eslint-disable-line max-len
const input = `0x${sourceAddress.slice(2)}${destinationAddress.slice(2)}${web3Utils.padLeft(value.toString("16"), "64", "0")}${data.slice(
2
)}${web3Utils.padLeft(nonce.toString("16"), "64", "0")}`; // eslint-disable-line max-len
const sigV = [];
const sigR = [];
const sigS = [];
const msgHash = web3Utils.soliditySha3(input);

for (let i = 0; i < signers.length; i += 1) {
const user = signers[i].toString();
const privKey = accountsJson.private_keys[user];
const prefixedMessageHash = ethUtils.hashPersonalMessage(Buffer.from(msgHash.slice(2), "hex"));
const sig = ethUtils.ecsign(prefixedMessageHash, Buffer.from(privKey, "hex"));

sigV.push(sig.v);
sigR.push(`0x${sig.r.toString("hex")}`);
sigS.push(`0x${sig.s.toString("hex")}`);
}

return { sigV, sigR, sigS };
}

export async function createSignaturesTrezor(colony, signers, value, data) {
const sourceAddress = colony.address;
const destinationAddress = colony.address;
const nonce = await colony.getTaskChangeNonce.call();
const accountsJson = JSON.parse(fs.readFileSync("./test-accounts.json", "utf8"));
const input = `0x${sourceAddress.slice(2)}${destinationAddress.slice(2)}${web3Utils.padLeft(value.toString("16"), "64", "0")}${data.slice(
2
)}${web3Utils.padLeft(nonce.toString("16"), "64", "0")}`; // eslint-disable-line max-len
const sigV = [];
const sigR = [];
const sigS = [];
const msgHash = web3Utils.soliditySha3(input);

for (let i = 0; i < signers.length; i += 1) {
const user = signers[i].toString();
const privKey = accountsJson.private_keys[user];
const sig = ethUtils.ecsign(Buffer.from(msgHash.slice(2), "hex"), Buffer.from(privKey, "hex"));

const prefixedMessageHash = web3Utils.soliditySha3("\x19Ethereum Signed Message:\n\x20", msgHash);
const sig = ethUtils.ecsign(Buffer.from(prefixedMessageHash.slice(2), "hex"), Buffer.from(privKey, "hex"));
sigV.push(sig.v);
sigR.push(`0x${sig.r.toString("hex")}`);
sigS.push(`0x${sig.s.toString("hex")}`);
Expand Down
80 changes: 66 additions & 14 deletions test/colony.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import {
checkErrorRevert,
expectEvent,
currentBlockTime,
createSignatures
createSignatures,
createSignaturesTrezor
} from "../helpers/test-helper";
import { fundColonyWithTokens, setupRatedTask, setupAssignedTask, setupFundedTask } from "../helpers/test-data-generator";

Expand Down Expand Up @@ -299,7 +300,7 @@ contract("Colony", addresses => {
let txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH_UPDATED);
const signers = [MANAGER, WORKER];
let sigs = await createSignatures(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

let taskChangeNonce = await colony.getTaskChangeNonce.call();
assert.equal(taskChangeNonce, 1);
Expand All @@ -308,7 +309,7 @@ contract("Colony", addresses => {
const dueDate = await currentBlockTime();
txData = await colony.contract.setTaskDueDate.getData(1, dueDate);
sigs = await createSignatures(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

taskChangeNonce = await colony.getTaskChangeNonce.call();
assert.equal(taskChangeNonce, 2);
Expand All @@ -320,11 +321,62 @@ contract("Colony", addresses => {
const txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH_UPDATED);
const signers = [MANAGER, WORKER];
const sigs = await createSignatures(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);
const task = await colony.getTask.call(1);
assert.equal(hexToUtf8(task[0]), SPECIFICATION_HASH_UPDATED);
});

it("should allow update of task brief signed by manager and worker using Trezor-style signatures", async () => {
await colony.makeTask(SPECIFICATION_HASH, 1);
await colony.setTaskRoleUser(1, WORKER_ROLE, WORKER);
const txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH_UPDATED);
const signers = [MANAGER, WORKER];
const sigs = await createSignaturesTrezor(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [1, 1], 0, txData);
const task = await colony.getTask.call(1);
assert.equal(hexToUtf8(task[0]), SPECIFICATION_HASH_UPDATED);
});

it("should allow update of task brief signed by manager and worker if one uses Trezor-style signatures and the other does not", async () => {
await colony.makeTask(SPECIFICATION_HASH, 1);
await colony.setTaskRoleUser(1, WORKER_ROLE, WORKER);
const txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH_UPDATED);
const signers = [MANAGER, WORKER];
const sigs = await createSignatures(colony, signers, 0, txData);
const trezorSigs = await createSignaturesTrezor(colony, signers, 0, txData);
await colony.executeTaskChange(
[sigs.sigV[0], trezorSigs.sigV[1]],
[sigs.sigR[0], trezorSigs.sigR[1]],
[sigs.sigS[0], trezorSigs.sigS[1]],
[0, 1],
0,
txData
);
const task = await colony.getTask.call(1);
assert.equal(hexToUtf8(task[0]), SPECIFICATION_HASH_UPDATED);
});

it("should not allow update of task brief signed by manager twice, with two different signature styles", async () => {
await colony.makeTask(SPECIFICATION_HASH, 1);
await colony.setTaskRoleUser(1, WORKER_ROLE, WORKER);
const txData = await colony.contract.setTaskBrief.getData(1, SPECIFICATION_HASH_UPDATED);
const signers = [MANAGER, WORKER];
const sigs = await createSignatures(colony, signers, 0, txData);
const trezorSigs = await createSignaturesTrezor(colony, signers, 0, txData);
await checkErrorRevert(
colony.executeTaskChange(
[sigs.sigV[0], trezorSigs.sigV[0]],
[sigs.sigR[0], trezorSigs.sigR[0]],
[sigs.sigS[0], trezorSigs.sigS[0]],
[0, 1],
0,
txData
)
);
const task = await colony.getTask.call(1);
assert.equal(hexToUtf8(task[0]), SPECIFICATION_HASH);
});

it("should allow update of task due date signed by manager and worker", async () => {
const dueDate = await currentBlockTime();

Expand All @@ -333,7 +385,7 @@ contract("Colony", addresses => {
const txData = await colony.contract.setTaskDueDate.getData(1, dueDate);
const signers = [MANAGER, WORKER];
const sigs = await createSignatures(colony, signers, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData);

const task = await colony.getTask.call(1);
assert.equal(task[4], dueDate);
Expand All @@ -352,7 +404,7 @@ contract("Colony", addresses => {
const signers = [MANAGER, OTHER];
const sigs = await createSignatures(colony, signers, 0, txData);

await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData));
await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData));
});

it("should fail update of task brief signed by manager and evaluator", async () => {
Expand All @@ -364,7 +416,7 @@ contract("Colony", addresses => {
const signers = [MANAGER, EVALUATOR];
const sigs = await createSignatures(colony, signers, 0, txData);

await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData));
await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData));
});

it("should fail to execute task change for a non-registered function signature", async () => {
Expand All @@ -373,7 +425,7 @@ contract("Colony", addresses => {
const signers = [MANAGER, EVALUATOR];
const sigs = await createSignatures(colony, signers, 0, txData);

await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData));
await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData));
});

it("should fail to execute change of task brief, using an invalid task id", async () => {
Expand All @@ -382,7 +434,7 @@ contract("Colony", addresses => {
const signers = [MANAGER, EVALUATOR];
const sigs = await createSignatures(colony, signers, 0, txData);

await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData));
await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData));
});

it("should fail to execute task change, if the task is already finalized", async () => {
Expand All @@ -394,7 +446,7 @@ contract("Colony", addresses => {
const signers = [MANAGER, EVALUATOR];
const sigs = await createSignatures(colony, signers, 0, txData);

await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData));
await checkErrorRevert(colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData));
});
});

Expand Down Expand Up @@ -581,21 +633,21 @@ contract("Colony", addresses => {
// Set the evaluator payout as 1000 ethers
const txData1 = await colony.contract.setTaskEvaluatorPayout.getData(1, 0x0, 1000);
const sigs = await createSignatures(colony, [MANAGER, EVALUATOR], 0, txData1);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, 0, txData1);
await colony.executeTaskChange(sigs.sigV, sigs.sigR, sigs.sigS, [0, 0], 0, txData1);

// Set the evaluator payout as 40 colony tokens
const txData2 = await colony.contract.setTaskEvaluatorPayout.getData(1, token.address, 40);
const sigs2 = await createSignatures(colony, [MANAGER, EVALUATOR], 0, txData2);
await colony.executeTaskChange(sigs2.sigV, sigs2.sigR, sigs2.sigS, 0, txData2);
await colony.executeTaskChange(sigs2.sigV, sigs2.sigR, sigs2.sigS, [0, 0], 0, txData2);

// Set the worker payout as 98000 wei and 200 colony tokens
const txData3 = await colony.contract.setTaskWorkerPayout.getData(1, 0x0, 98000);
const sigs3 = await createSignatures(colony, [MANAGER, WORKER], 0, txData3);
await colony.executeTaskChange(sigs3.sigV, sigs3.sigR, sigs3.sigS, 0, txData3);
await colony.executeTaskChange(sigs3.sigV, sigs3.sigR, sigs3.sigS, [0, 0], 0, txData3);

const txData4 = await colony.contract.setTaskWorkerPayout.getData(1, token.address, 200);
const sigs4 = await createSignatures(colony, [MANAGER, WORKER], 0, txData4);
await colony.executeTaskChange(sigs4.sigV, sigs4.sigR, sigs4.sigS, 0, txData4);
await colony.executeTaskChange(sigs4.sigV, sigs4.sigR, sigs4.sigS, [0, 0], 0, txData4);

const taskPayoutManager1 = await colony.getTaskPayout.call(1, MANAGER_ROLE, 0x0);
assert.equal(taskPayoutManager1.toNumber(), 5000);
Expand Down