From 7e76178c17cff1ce232f82d0e0abe36c04b9f10e Mon Sep 17 00:00:00 2001 From: justinbretting Date: Fri, 4 Jan 2019 13:52:06 -0700 Subject: [PATCH 1/2] patch getNextSequenceId to always return a usable seq id --- contracts/WalletSimple.sol | 69 ++++++++++++++++++-------------- test/walletsimple.js | 81 ++++++++++++++++++++++++++++++-------- 2 files changed, 103 insertions(+), 47 deletions(-) diff --git a/contracts/WalletSimple.sol b/contracts/WalletSimple.sol index 1edebf8..8bd7237 100644 --- a/contracts/WalletSimple.sol +++ b/contracts/WalletSimple.sol @@ -1,4 +1,5 @@ pragma solidity ^0.4.18; + import "./Forwarder.sol"; import "./ERC20Interface.sol"; /** @@ -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. @@ -63,7 +64,6 @@ contract WalletSimple { } signers = allowedSigners; } - /** * Determine if an address is a signer on this wallet * @param signer address to check @@ -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 */ @@ -88,7 +87,6 @@ contract WalletSimple { } _; } - /** * Gets called when a transaction is received without calling a method */ @@ -98,7 +96,6 @@ 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 @@ -106,7 +103,6 @@ contract WalletSimple { 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. @@ -128,9 +124,7 @@ 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 @@ -138,7 +132,7 @@ contract WalletSimple { } 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. @@ -160,15 +154,14 @@ 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 * @@ -176,13 +169,12 @@ contract WalletSimple { * @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 * @@ -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! @@ -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(); @@ -225,10 +213,8 @@ 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. */ @@ -236,7 +222,6 @@ contract WalletSimple { safeMode = true; SafeModeActivated(msg.sender); } - /** * Gets signer's address using ecrecover * @param operationHash see Data Formats @@ -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 @@ -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; } } diff --git a/test/walletsimple.js b/test/walletsimple.js index f90a1f0..0c73107 100644 --- a/test/walletsimple.js +++ b/test/walletsimple.js @@ -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; @@ -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] }); @@ -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] }); @@ -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' + }; + + 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 + }; + + expectSuccessfulSendMultiSig(params); + }); + }) + describe('Safe mode', function() { before(async function() { // Create and fund the wallet @@ -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); @@ -953,4 +1001,3 @@ contract('WalletSimple', function(accounts) { }); }); - From 224c5efcc8b400f47543c20da39117c5b0397463 Mon Sep 17 00:00:00 2001 From: justinbretting Date: Fri, 4 Jan 2019 14:04:37 -0700 Subject: [PATCH 2/2] await successful sends in seq id tests --- test/walletsimple.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/walletsimple.js b/test/walletsimple.js index 0c73107..9fe7da5 100644 --- a/test/walletsimple.js +++ b/test/walletsimple.js @@ -721,7 +721,7 @@ contract('WalletSimple', function(accounts) { sequenceId: '10000' }; - expectSuccessfulSendMultiSig(params); + await expectSuccessfulSendMultiSig(params); }); it('still returns a usable next sequence ID after tx with seq id 10,000 higher than lowest in recentSequenceIds', async () => { @@ -740,7 +740,7 @@ contract('WalletSimple', function(accounts) { sequenceId: sequenceId }; - expectSuccessfulSendMultiSig(params); + await expectSuccessfulSendMultiSig(params); }); })