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

Usability issue with sequence ids #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Usability issue with sequence ids #11

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
7e76178
patch getNextSequenceId to always return a usable seq id
justinbretting Jan 4, 2019
224c5ef
await successful sends in seq id tests
justinbretting Jan 4, 2019
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
69 changes: 39 additions & 30 deletions contracts/WalletSimple.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pragma solidity ^0.4.18;

import "./Forwarder.sol";
import "./ERC20Interface.sol";
/**
Expand Down Expand Up @@ -39,14 +40,14 @@ contract WalletSimple {
uint value, // Amount of Wei sent to the address
bytes data // Data sent when invoking the transaction
);

// Public fields
address[] public signers; // The addresses that can co-sign transactions on the wallet
bool public safeMode = false; // When active, wallet may only send to signer addresses
// make public so they can be read in case something goes sideways with sequence ids
uint[10] public recentSequenceIds;

// Internal fields
uint constant SEQUENCE_ID_WINDOW_SIZE = 10;
uint[10] recentSequenceIds;

/**
* Set up a simple multi-sig wallet by specifying the signers allowed to be used on this wallet.
Expand All @@ -63,7 +64,6 @@ contract WalletSimple {
}
signers = allowedSigners;
}

/**
* Determine if an address is a signer on this wallet
* @param signer address to check
Expand All @@ -78,7 +78,6 @@ contract WalletSimple {
}
return false;
}

/**
* Modifier that will execute internal code block only if the sender is an authorized signer on this wallet
*/
Expand All @@ -88,7 +87,6 @@ contract WalletSimple {
}
_;
}

/**
* Gets called when a transaction is received without calling a method
*/
Expand All @@ -98,15 +96,13 @@ contract WalletSimple {
Deposited(msg.sender, msg.value, msg.data);
}
}

/**
* Create a new contract (and also address) that forwards funds to this contract
* returns address of newly created forwarder address
*/
function createForwarder() public returns (address) {
return new Forwarder();
}

/**
* Execute a multi-signature transaction from this wallet using 2 signers: one from msg.sender and the other from ecrecover.
* Sequence IDs are numbers starting from 1. They are used to prevent replay attacks and may not be repeated.
Expand All @@ -128,17 +124,15 @@ contract WalletSimple {
) public onlySigner {
// Verify the other signer
var operationHash = keccak256("ETHER", toAddress, value, data, expireTime, sequenceId);

var otherSigner = verifyMultiSig(toAddress, operationHash, signature, expireTime, sequenceId);

// Success, send the transaction
if (!(toAddress.call.value(value)(data))) {
// Failed executing transaction
revert();
}
Transacted(msg.sender, otherSigner, operationHash, toAddress, value, data);
}

/**
* Execute a multi-signature token transfer from this wallet using 2 signers: one from msg.sender and the other from ecrecover.
* Sequence IDs are numbers starting from 1. They are used to prevent replay attacks and may not be repeated.
Expand All @@ -160,29 +154,27 @@ contract WalletSimple {
) public onlySigner {
// Verify the other signer
var operationHash = keccak256("ERC20", toAddress, value, tokenContractAddress, expireTime, sequenceId);

verifyMultiSig(toAddress, operationHash, signature, expireTime, sequenceId);

ERC20Interface instance = ERC20Interface(tokenContractAddress);
if (!instance.transfer(toAddress, value)) {
revert();
}
}

/**
* Execute a token flush from one of the forwarder addresses. This transfer needs only a single signature and can be done by any signer
*
* @param forwarderAddress the address of the forwarder address to flush the tokens from
* @param tokenContractAddress the address of the erc20 token contract
*/
function flushForwarderTokens(
address forwarderAddress,
address forwarderAddress,
address tokenContractAddress
) public onlySigner {
Forwarder forwarder = Forwarder(forwarderAddress);
forwarder.flushTokens(tokenContractAddress);
}

/**
* Do common multisig verification for both eth sends and erc20token transfers
*
Expand All @@ -200,9 +192,7 @@ contract WalletSimple {
uint expireTime,
uint sequenceId
) private returns (address) {

var otherSigner = recoverAddressFromSignature(operationHash, signature);

// Verify if we are in safe mode. In safe mode, the wallet can only send to signers
if (safeMode && !isSigner(toAddress)) {
// We are in safe mode and the toAddress is not a signer. Disallow!
Expand All @@ -213,10 +203,8 @@ contract WalletSimple {
// Transaction expired
revert();
}

// Try to insert the sequence ID. Will revert if the sequence id was invalid
tryInsertSequenceId(sequenceId);

if (!isSigner(otherSigner)) {
// Other signer not on this wallet or operation does not match arguments
revert();
Expand All @@ -225,18 +213,15 @@ contract WalletSimple {
// Cannot approve own transaction
revert();
}

return otherSigner;
}

/**
* Irrevocably puts contract into safe mode. When in this mode, transactions may only be sent to signing addresses.
*/
function activateSafeMode() public onlySigner {
safeMode = true;
SafeModeActivated(msg.sender);
}

/**
* Gets signer's address using ecrecover
* @param operationHash see Data Formats
Expand Down Expand Up @@ -264,7 +249,6 @@ contract WalletSimple {
}
return ecrecover(operationHash, v, r, s);
}

/**
* Verify that the sequence id has not been used before and inserts it. Throws if the sequence ID was not accepted.
* We collect a window of up to 10 recent sequence ids, and allow any sequence id that is not in the window and
Expand Down Expand Up @@ -295,18 +279,43 @@ contract WalletSimple {
}
recentSequenceIds[lowestValueIndex] = sequenceId;
}

/**
* Gets the next available sequence ID for signing when using executeAndConfirm
* returns the sequenceId one higher than the highest currently stored
* Gets the next available sequence ID for signing.
* Returns the sequenceId one higher than the highest value
* in the lowest sequence
*
* IE, if recentSequenceIds == [0,100,200,40,75,10,1,4,2,5], return 3
*/
function getNextSequenceId() public view returns (uint) {
uint highestSequenceId = 0;
// first, find the lowest value index
uint lowestValueIndex = 0;
for (uint i = 0; i < SEQUENCE_ID_WINDOW_SIZE; i++) {
if (recentSequenceIds[i] > highestSequenceId) {
highestSequenceId = recentSequenceIds[i];
if (recentSequenceIds[i] < recentSequenceIds[lowestValueIndex]) {
lowestValueIndex = i;
}
}
return highestSequenceId + 1;

// then, iterate through the sequenceIds until it finds an available one
uint nextSequenceId = recentSequenceIds[lowestValueIndex] + 1;
bool found = false;

for (uint j = 0; j <= SEQUENCE_ID_WINDOW_SIZE; j++) {
// zero points for elegance
found = false;
for (uint k = 0; k < SEQUENCE_ID_WINDOW_SIZE; k++) {
if (nextSequenceId == recentSequenceIds[k]) {
found = true;
}
}

// if it's not in the array, it's an available slot
if (!found) {
break;
}

nextSequenceId++;
}

return nextSequenceId;
}
}
81 changes: 64 additions & 17 deletions test/walletsimple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ const createForwarderFromWallet = async (wallet) => {
return Forwarder.at(forwarderAddress);
};

const getSequenceId = async function(wallet) {
const sequenceIdString = await wallet.getNextSequenceId.call();
return parseInt(sequenceIdString);
};

contract('WalletSimple', function(accounts) {
let wallet;
let walletEvents;
Expand Down Expand Up @@ -158,21 +163,16 @@ contract('WalletSimple', function(accounts) {
wallet = await WalletSimple.new([accounts[0], accounts[1], accounts[2]]);
});

const getSequenceId = async function() {
const sequenceIdString = await wallet.getNextSequenceId.call();
return parseInt(sequenceIdString);
};

it('Authorized signer can request and insert an id', async function() {
let sequenceId = await getSequenceId();
let sequenceId = await getSequenceId(wallet);
sequenceId.should.eql(1);
await wallet.tryInsertSequenceId(sequenceId, { from: accounts[0] });
sequenceId = await getSequenceId();
sequenceId = await getSequenceId(wallet);
sequenceId.should.eql(2);
});

it('Non-signer cannot insert an id', async function() {
const sequenceId = await getSequenceId();
const sequenceId = await getSequenceId(wallet);

try {
await wallet.tryInsertSequenceId(sequenceId, { from: accounts[8] });
Expand All @@ -182,37 +182,37 @@ contract('WalletSimple', function(accounts) {
}

// should be unchanged
const newSequenceId = await getSequenceId();
const newSequenceId = await getSequenceId(wallet);
sequenceId.should.eql(newSequenceId);
});

it('Can request large sequence ids', async function() {
for (let i=0; i<30; i++) {
let sequenceId = await getSequenceId();
let sequenceId = await getSequenceId(wallet);
// Increase by 100 each time to test for big numbers (there will be holes, this is ok)
sequenceId += 100;
await wallet.tryInsertSequenceId(sequenceId, { from: accounts[0] });
const newSequenceId = await getSequenceId();
const newSequenceId = await getSequenceId(wallet);
newSequenceId.should.eql(sequenceId + 1);
}
});

it('Can request lower but unused recent sequence id within the window', async function() {
const windowSize = 10;
let sequenceId = await getSequenceId();
let sequenceId = await getSequenceId(wallet);
const originalNextSequenceId = sequenceId;
// Try for 9 times (windowsize - 1) because the last window was used already
for (let i=0; i < (windowSize - 1); i++) {
sequenceId -= 5; // since we were incrementing 100 per time, this should be unused
await wallet.tryInsertSequenceId(sequenceId, { from: accounts[0] });
}
const newSequenceId = await getSequenceId();
const newSequenceId = await getSequenceId(wallet);
// we should still get the same next sequence id since we were using old ids
newSequenceId.should.eql(originalNextSequenceId);
});

it('Cannot request lower but used recent sequence id within the window', async function() {
let sequenceId = await getSequenceId();
let sequenceId = await getSequenceId(wallet);
sequenceId -= 50; // we used this in the previous test
try {
await wallet.tryInsertSequenceId(sequenceId, { from: accounts[8] });
Expand Down Expand Up @@ -696,6 +696,54 @@ contract('WalletSimple', function(accounts) {
});
});

describe('Get Next Sequence Id', function() {
before(async function() {
// Create and fund the wallet
wallet = await WalletSimple.new([accounts[0], accounts[1], accounts[2]]);
web3.eth.sendTransaction({ from: accounts[0], to: wallet.address, value: web3.toWei(200000, 'ether') });
web3.fromWei(web3.eth.getBalance(wallet.address), 'ether').should.eql(web3.toBigNumber(200000));
});

it('returns 1 for first sequence number', async () => {
const sequenceId = await getSequenceId(wallet);
sequenceId.should.eql(1);
});

it('can send with exactly 10,000 higher than lowest sequence number', async () => {
const params = {
msgSenderAddress: accounts[2],
otherSignerAddress: accounts[1],
wallet: wallet,
toAddress: accounts[5],
amount: 62,
data: '',
expireTime: Math.floor((new Date().getTime()) / 1000) + 60,
sequenceId: '10000'
};

await expectSuccessfulSendMultiSig(params);
});

it('still returns a usable next sequence ID after tx with seq id 10,000 higher than lowest in recentSequenceIds', async () => {
const sequenceId = await getSequenceId(wallet);

sequenceId.should.eql(1);

const params = {
msgSenderAddress: accounts[2],
otherSignerAddress: accounts[1],
wallet: wallet,
toAddress: accounts[5],
amount: 62,
data: '',
expireTime: Math.floor((new Date().getTime()) / 1000) + 60,
sequenceId: sequenceId
};

await expectSuccessfulSendMultiSig(params);
});
})

describe('Safe mode', function() {
before(async function() {
// Create and fund the wallet
Expand Down Expand Up @@ -895,13 +943,13 @@ contract('WalletSimple', function(accounts) {
});

it('Receive and Send tokens from main wallet contract', async function() {

await fixedSupplyTokenContract.transfer(wallet.address, 100, { from: accounts[0] });
const balance = await fixedSupplyTokenContract.balanceOf.call(accounts[0]);
balance.should.eql(web3.toBigNumber(1000000 - 100));
const msigWalletStartTokens = await fixedSupplyTokenContract.balanceOf.call(wallet.address);
msigWalletStartTokens.should.eql(web3.toBigNumber(100));

const sequenceIdString = await wallet.getNextSequenceId.call();
const sequenceId = parseInt(sequenceIdString);

Expand Down Expand Up @@ -953,4 +1001,3 @@ contract('WalletSimple', function(accounts) {
});

});