Skip to content

Commit

Permalink
Updated parameter orders for shiftIn and hashForSignature
Browse files Browse the repository at this point in the history
  • Loading branch information
@0x31
0x31 committed Jun 24, 2019
1 parent c44a4f4 commit 3ef03f6
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 61 deletions.
31 changes: 17 additions & 14 deletions contracts/Shifter/Shifter.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,22 @@ contract Shifter is Ownable {

/// @notice shiftIn mints tokens after taking a fee for the `_feeRecipient`.
///
/// @param _pHash (payload hash) The hash of the payload associated with the
/// shift.
/// @param _amount The amount of the token being shifted int, in its
/// smallest value. (e.g. satoshis for BTC)
/// @param _nHash (nonce hash) The hash of the nonce, amount and pHash.
/// @param _pHash (payload hash) The hash of the payload associated with the
/// shift.
/// @param _sig The signature of the hash of the following values:
/// (msg.sender, amount, nHash, pHash), signed by the mintAuthority.
function shiftIn(uint256 _amount, bytes32 _nHash, bytes memory _sig, bytes32 _pHash) public returns (uint256) {
return _shiftIn(msg.sender, _amount, _nHash, _sig, _pHash);
/// (pHash, amount, msg.sender, nHash), signed by the mintAuthority.
function shiftIn(bytes32 _pHash, uint256 _amount, bytes32 _nHash, bytes memory _sig) public returns (uint256) {
// Use msg.sender as the _to address
return _shiftIn(_pHash, _amount, msg.sender, _nHash, _sig);
}

/// @notice Callable by the previous Shifter if it has been upgraded.
function forwardShiftIn(address _to, uint256 _amount, bytes32 _nHash, bytes memory _sig, bytes32 _pHash) public returns (uint256) {
function forwardShiftIn(bytes32 _pHash, uint256 _amount, address _to, bytes32 _nHash, bytes memory _sig) public returns (uint256) {
require(authorizedWrapper[msg.sender], "not authorized to mint on behalf of user");
return _shiftIn(_to, _amount, _nHash, _sig, _pHash);
return _shiftIn(_pHash, _amount, _to, _nHash, _sig);
}

/// @notice shiftOut burns tokens after taking a fee for the `_feeRecipient`.
Expand Down Expand Up @@ -145,27 +146,28 @@ contract Shifter is Ownable {
}

/// @notice hashForSignature hashes the parameters so that they can be signed.
function hashForSignature(address _to, uint256 _amount, bytes32 _nHash, bytes32 _pHash) public view returns (bytes32) {
function hashForSignature(bytes32 _pHash, uint256 _amount, address _to, bytes32 _nHash) public view returns (bytes32) {
// Check if the contract has been upgraded and forward the call
if (nextShifter != address(0x0)) {
return Shifter(nextShifter).hashForSignature(_to, _amount, _nHash, _pHash);
return Shifter(nextShifter).hashForSignature(_pHash, _amount, _to, _nHash);
}

return keccak256(abi.encode(address(token), _to, _amount, _nHash, _pHash));
return keccak256(abi.encode(_pHash, _amount, address(token), _to, _nHash));
}

// Internal functions //////////////////////////////////////////////////////

/// @notice shiftIn mints new tokens after verifying the signature and
/// @notice _shiftIn mints new tokens after verifying the signature and
/// transfers the tokens to `_to`.
function _shiftIn(address _to, uint256 _amount, bytes32 _nHash, bytes memory _sig, bytes32 _pHash) internal returns (uint256) {
/// The `_to` parameter is set by `shiftIn` or `forwardShiftIn`.
function _shiftIn(bytes32 _pHash, uint256 _amount, address _to, bytes32 _nHash, bytes memory _sig) internal returns (uint256) {
// Check if the contract has been upgraded and forward the call
if (nextShifter != address(0x0)) {
return Shifter(nextShifter).forwardShiftIn(_to, _amount, _nHash, _sig, _pHash);
return Shifter(nextShifter).forwardShiftIn(_pHash, _amount, _to, _nHash, _sig);
}

// Verify signature
bytes32 signedMessageHash = hashForSignature(_to, _amount, _nHash, _pHash);
bytes32 signedMessageHash = hashForSignature(_pHash, _amount, _to, _nHash);
require(status[signedMessageHash] == false, "nonce hash already spent");
require(verifySignature(signedMessageHash, _sig), "invalid signature");
status[signedMessageHash] = true;
Expand All @@ -183,6 +185,7 @@ contract Shifter is Ownable {
return receivedAmount;
}

/// The `_from` parameter is set by `shiftOut` or `forwardShiftOut`.
function _shiftOut(address _from, bytes memory _to, uint256 _amount) internal returns (uint256) {
// Check if the contract has been upgraded and forward the call
if (nextShifter != address(0x0)) {
Expand Down
12 changes: 7 additions & 5 deletions contracts/Shifter/examples/Vesting.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,14 @@ contract Vesting is Ownable {
/// period should begin.
/// @param _duration The number of months for the vesting period.
function addVestingSchedule(
uint256 _amount,
bytes32 _nHash,
bytes calldata _sig,
// Payload
address _beneficiary,
uint256 _startTime,
uint16 _duration
uint16 _duration,
// Required
uint256 _amount,
bytes32 _nHash,
bytes calldata _sig
) external onlyOwner {
require(schedules[_beneficiary].startTime == 0, "vesting schedule already exists");
require(_amount > 0, "amount must be greater than 0");
Expand All @@ -80,7 +82,7 @@ contract Vesting is Ownable {
// contract. This will verify the signature to ensure the Darknodes have
// received the Bitcoin.
bytes32 pHash = keccak256(abi.encode(_beneficiary, _startTime, _duration));
btc.shiftIn(_amount, _nHash, _sig, pHash);
btc.shiftIn(pHash, _amount, _nHash, _sig);
}

/// @notice Allows a beneficiary to withdraw their vested Bitcoin.
Expand Down
72 changes: 36 additions & 36 deletions test/Shifter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { keccak256 } from "web3-utils";

import { BTCShifterInstance, ShifterInstance, zBTCInstance } from "../types/truffle-contracts";
import { log } from "./helper/logs";
import { increaseTime, NULL } from "./helper/testUtils";
import { increaseTime, NULL, Ox } from "./helper/testUtils";

const ShifterRegistry = artifacts.require("ShifterRegistry");
const BTCShifter = artifacts.require("BTCShifter");
Expand Down Expand Up @@ -43,24 +43,24 @@ contract("Shifter", ([defaultAcc, feeRecipient, user, malicious]) => {
const removeFee = (value, bips) => value.sub(value.mul(new BN(bips)).div(new BN(10000)))

const mintTest = async (shifter: ShifterInstance, value: BN, shiftID = undefined) => {
const nHash = `0x${randomBytes(32).toString("hex")}`;
const pHash = `0x${randomBytes(32).toString("hex")}`;
const nHash = Ox(randomBytes(32).toString("hex"));
const pHash = Ox(randomBytes(32).toString("hex"));

const hash = await shifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hash = await shifter.hashForSignature(pHash, value.toNumber(), user, nHash);
const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);

pubToAddress(ecrecover(Buffer.from(hash.slice(2), "hex"), sig.v, sig.r, sig.s)).toString("hex")
.should.equal(mintAuthority.address.slice(2).toLowerCase());

const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

const hashForSignature = await shifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hashForSignature = await shifter.hashForSignature(pHash, value.toNumber(), user, nHash);
(await shifter.verifySignature(hashForSignature, sigString))
.should.be.true;

const balanceBefore = new BN((await zbtc.balanceOf(user)).toString());
const _shiftID = await shifter.nextShiftID();
(await shifter.shiftIn(value.toNumber(), nHash, sigString, pHash, { from: user }) as any)
(await shifter.shiftIn(pHash, value.toNumber(), nHash, sigString, { from: user }) as any)
.should.emit.logs([
log("LogShiftIn", { _to: user, _amount: removeFee(value, 10), _shiftID: shiftID !== undefined ? shiftID : _shiftID }),
]);
Expand All @@ -71,7 +71,7 @@ contract("Shifter", ([defaultAcc, feeRecipient, user, malicious]) => {

const burnTest = async (shifter: ShifterInstance, value: BN, btcAddress?: string, shiftID = undefined) => {
// Note: we don't use `||` because we want to pass in `""`
btcAddress = btcAddress !== undefined ? btcAddress : `0x${randomBytes(35).toString("hex")}`;
btcAddress = btcAddress !== undefined ? btcAddress : Ox(randomBytes(35).toString("hex"));

const balanceBefore = new BN((await zbtc.balanceOf(user)).toString());
const _shiftID = await shifter.nextShiftID();
Expand All @@ -89,59 +89,59 @@ contract("Shifter", ([defaultAcc, feeRecipient, user, malicious]) => {
it("won't mint for the same nHash and pHash twice", async () => {
const [pHash, nHash] = await mintTest(btcShifter, value);

const hash = await btcShifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash);
const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);
const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

await btcShifter.shiftIn(value.toNumber(), nHash, sigString, pHash, { from: user })
await btcShifter.shiftIn(pHash, value.toNumber(), nHash, sigString, { from: user })
.should.be.rejectedWith(/nonce hash already spent/);
});

it("can mint for the same pHash with a different nHash", async () => {
const [pHash, _] = await mintTest(btcShifter, value);

const nHash = `0x${randomBytes(32).toString("hex")}`;
const nHash = Ox(randomBytes(32).toString("hex"));

const hash = await btcShifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash);
const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);
const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

const balanceBefore = new BN((await zbtc.balanceOf(user)).toString());
await btcShifter.shiftIn(value.toNumber(), nHash, sigString, pHash, { from: user });
await btcShifter.shiftIn(pHash, value.toNumber(), nHash, sigString, { from: user });
(await zbtc.balanceOf(user)).should.bignumber.equal(balanceBefore.add(removeFee(value, 10)));

await burnTest(btcShifter, removeFee(value, 10));
});

it("won't mind with an invalid signature", async () => {
const nHash1 = `0x${randomBytes(32).toString("hex")}`;
const nHash2 = `0x${randomBytes(32).toString("hex")}`;
const pHash = `0x${randomBytes(32).toString("hex")}`;
it("won't mint with an invalid signature", async () => {
const nHash1 = Ox(randomBytes(32).toString("hex"));
const nHash2 = Ox(randomBytes(32).toString("hex"));
const pHash = Ox(randomBytes(32).toString("hex"));

const hash = await btcShifter.hashForSignature(user, value.toNumber(), nHash1, pHash);
const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash1);
const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);

const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

await btcShifter.shiftIn(value.toNumber(), nHash2, sigString, pHash, { from: user })
await btcShifter.shiftIn(pHash, value.toNumber(), nHash2, sigString, { from: user })
.should.be.rejectedWith(/invalid signature/);
});

it("can't call forwardShiftIn", async () => {
const nHash = `0x${randomBytes(32).toString("hex")}`;
const pHash = `0x${randomBytes(32).toString("hex")}`;
const nHash = Ox(randomBytes(32).toString("hex"));
const pHash = Ox(randomBytes(32).toString("hex"));

const hash = await btcShifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash);
const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);

const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

await btcShifter.forwardShiftIn(user, value.toNumber(), nHash, sigString, pHash, { from: malicious })
await btcShifter.forwardShiftIn(pHash, value.toNumber(), user, nHash, sigString, { from: malicious })
.should.be.rejectedWith(/not authorized to mint on behalf of user/);
});

it("can't call forwardShiftOut", async () => {
const btcAddress = `0x${randomBytes(35).toString("hex")}`;
const btcAddress = Ox(randomBytes(35).toString("hex"));
await btcShifter.forwardShiftOut(user, btcAddress, removeFee(value, 10).toNumber(), { from: malicious })
.should.be.rejectedWith(/not authorized to burn on behalf of user/);
});
Expand All @@ -160,10 +160,10 @@ contract("Shifter", ([defaultAcc, feeRecipient, user, malicious]) => {
// mint twice. See "Signature Malleability" at
// https://yondon.blog/2019/01/01/how-not-to-use-ecdsa/

const nHash = `0x${randomBytes(32).toString("hex")}`;
const pHash = `0x${randomBytes(32).toString("hex")}`;
const nHash = Ox(randomBytes(32).toString("hex"));
const pHash = Ox(randomBytes(32).toString("hex"));

const hash = await btcShifter.hashForSignature(user, value.toNumber(), nHash, pHash);
const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash);

let sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);

Expand All @@ -173,18 +173,18 @@ contract("Shifter", ([defaultAcc, feeRecipient, user, malicious]) => {
s: new BN("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", "hex").sub(new BN(sig.s)).toArrayLike(Buffer, "be", 32),
v: sig.v === 27 ? 28 : 27,
};
const altSigString = `0x${altSig.r.toString("hex")}${altSig.s.toString("hex")}${(altSig.v).toString(16)}`;
await btcShifter.shiftIn(value.toNumber(), nHash, altSigString, pHash, { from: user })
const altSigString = Ox(`${altSig.r.toString("hex")}${altSig.s.toString("hex")}${(altSig.v).toString(16)}`);
await btcShifter.shiftIn(pHash, value.toNumber(), nHash, altSigString, { from: user })
.should.be.rejectedWith(/signature's s is in the wrong range/);

// Valid signature
const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
await btcShifter.shiftIn(value.toNumber(), nHash, sigString, pHash, { from: user });
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);
await btcShifter.shiftIn(pHash, value.toNumber(), nHash, sigString, { from: user });

// Using the invalid signature after the valid one should throw
// before checking the signature because the nonce hash has already
// been used
await btcShifter.shiftIn(value.toNumber(), nHash, altSigString, pHash, { from: user })
await btcShifter.shiftIn(pHash, value.toNumber(), nHash, altSigString, { from: user })
.should.be.rejectedWith(/nonce hash already spent/);
});
});
Expand Down
15 changes: 10 additions & 5 deletions test/Vesting.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import BigNumber from "bignumber.js";
import { rawEncode } from "ethereumjs-abi";

import { BTCShifterInstance, VestingInstance, zBTCInstance } from "../types/truffle-contracts";
import { increaseTime, NULL } from "./helper/testUtils";
import { increaseTime, NULL, Ox } from "./helper/testUtils";

const BTCShifter = artifacts.require("BTCShifter");
const zBTC = artifacts.require("zBTC");
Expand Down Expand Up @@ -47,23 +47,28 @@ contract("Vesting", (accounts) => {
const duration = 6;

const addVestingSchedule = async () => {
const nonce = `0x${randomBytes(32).toString("hex")}`;
const nonce = Ox(randomBytes(32).toString("hex"));

const startTime = 0;
const pHash = keccak256(rawEncode(
["address", "uint256", "uint16"],
[beneficiary, startTime, duration]
)).toString("hex");

const hashForSignature = await btcShifter.hashForSignature(vesting.address, amount.toNumber(), nonce, `0x${pHash}`);
const hashForSignature = await btcShifter.hashForSignature(Ox(pHash), amount.toNumber(), vesting.address, nonce);
const sig = ecsign(Buffer.from(hashForSignature.slice(2), "hex"), privKey);
const sigString = `0x${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`;
const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);

// User should have no schedules prior to adding.
let schedule = await vesting.schedules(beneficiary);
(schedule as any).startTime.should.bignumber.equal(new BN(0));

await vesting.addVestingSchedule(amount, nonce, sigString, beneficiary, startTime, duration);
await vesting.addVestingSchedule(
// Payload
beneficiary, startTime, duration,
// Required
amount, nonce, sigString,
);
}

it("can add a vesting schedule", async () => {
Expand Down
5 changes: 4 additions & 1 deletion test/helper/testUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,11 @@ export function PUBK(i: string | number) {
export const NULL = "0x0000000000000000000000000000000000000000";
export const NULL32 = "0x0000000000000000000000000000000000000000000000000000000000000000";

// Add a 0x prefix from a hex string
export const Ox = (hex: string) => hex.substring(0, 2) === "0x" ? hex : `0x${hex}`;

export const randomBytes = (bytes: number): string => {
return `0x${crypto.randomBytes(bytes).toString("hex")}`;
return Ox(crypto.randomBytes(bytes).toString("hex"));
};

export const randomAddress = (): string => {
Expand Down

0 comments on commit 3ef03f6

Please sign in to comment.