From 95ec40a9b4cabceef4559fa561e14cd16c6b4a18 Mon Sep 17 00:00:00 2001 From: Patricio Palladino Date: Thu, 25 Jul 2019 12:22:21 -0300 Subject: [PATCH 1/2] Validate storage keys' length and improve doc --- lib/state/stateManager.ts | 25 ++++++++++++--- tests/api/state/stateManager.js | 57 +++++++++++++++++++++++++++++---- 2 files changed, 71 insertions(+), 11 deletions(-) diff --git a/lib/state/stateManager.ts b/lib/state/stateManager.ts index 0828e30ef5..bd05b61f01 100644 --- a/lib/state/stateManager.ts +++ b/lib/state/stateManager.ts @@ -1,3 +1,5 @@ +import { keccak256 } from 'ethereumjs-util' + const Set = require('core-js-pure/es/set') const Trie = require('merkle-patricia-tree/secure.js') const asyncLib = require('async') @@ -194,12 +196,17 @@ export default class StateManager { */ /** - * Gets the storage value associated with the provided `address` and `key`. + * Gets the storage value associated with the provided `address` and `key`. This method returns + * the shortest representation of the stored value. * @param address - Address of the account to get the storage for - * @param key - Key in the account's storage to get the value for - * @param {getContractCode~callback} cb + * @param key - Key in the account's storage to get the value for. Must be 32 bytes long. + * @param {getContractCode~callback} cb. */ getContractStorage(address: Buffer, key: Buffer, cb: any): void { + if (key.length !== 32) { + return cb(new Error('Storage key must be 32 bytes long')) + } + this._getStorageTrie(address, (err: Error, trie: any) => { if (err) { return cb(err) @@ -220,9 +227,13 @@ export default class StateManager { * onwards. This is used to get the original value of a storage slot for * computing gas costs according to EIP-1283. * @param address - Address of the account to get the storage for - * @param key - Key in the account's storage to get the value for + * @param key - Key in the account's storage to get the value for. Must be 32 bytes long. */ getOriginalContractStorage(address: Buffer, key: Buffer, cb: any): void { + if (key.length !== 32) { + return cb(new Error('Storage key must be 32 bytes long')) + } + const addressHex = address.toString('hex') const keyHex = key.toString('hex') @@ -275,11 +286,15 @@ export default class StateManager { * Adds value to the state trie for the `account` * corresponding to `address` at the provided `key`. * @param address - Address to set a storage value for - * @param key - Key to set the value at + * @param key - Key to set the value at. Must be 32 bytes long. * @param value - Value to set at `key` for account corresponding to `address` * @param cb - Callback function */ putContractStorage(address: Buffer, key: Buffer, value: Buffer, cb: any): void { + if (key.length !== 32) { + return cb(new Error('Storage key must be 32 bytes long')) + } + this._modifyContractStorage( address, (storageTrie: any, done: any) => { diff --git a/tests/api/state/stateManager.js b/tests/api/state/stateManager.js index a27ba3de38..7fbf86340c 100644 --- a/tests/api/state/stateManager.js +++ b/tests/api/state/stateManager.js @@ -50,7 +50,7 @@ tape('StateManager', (t) => { // test contract storage cache await checkpoint() - const key = Buffer.from('0x1234') + const key = util.toBuffer('0x1234567890123456789012345678901234567890123456789012345678901234') const value = Buffer.from('0x1234') await putContractStorage(addressBuffer, key, value) @@ -150,12 +150,12 @@ tape('StateManager', (t) => { account ) - const key = Buffer.from('0x1234') - const value = Buffer.from('0x1234') + const key = util.toBuffer('0x1234567890123456789012345678901234567890123456789012345678901234') + const value = util.toBuffer('0x0a') // We used this value as its RLP encoding is also 0a await putContractStorage(addressBuffer, key, value) stateManager.dumpStorage(addressBuffer, (data) => { - const expect = { '1ac7d1b81b7ba1025b36ccb86723da6ee5a87259f1c2fd5abe69d3200b512ec8': '86307831323334' } + const expect = { [util.keccak256(key).toString('hex')]: '0a' } st.deepEqual(data, expect, 'should dump storage value') st.end() @@ -176,6 +176,38 @@ tape('StateManager', (t) => { st.end() }) + + t.test('should validate the key\'s length when modifying a contract\'s storage', async st => { + const stateManager = new StateManager() + const addressBuffer = Buffer.from('a94f5374fce5edbc8e2a8697c15331677e6ebf0b', 'hex') + const putContractStorage = promisify((...args) => stateManager.putContractStorage(...args)) + try { + await putContractStorage(addressBuffer, Buffer.alloc(12), util.toBuffer('0x1231')) + } catch (e) { + st.equal(e.message, 'Storage key must be 32 bytes long') + st.end() + return + } + + st.fail('Should have failed') + st.end() + }) + + t.test('should validate the key\'s length when reading a contract\'s storage', async st => { + const stateManager = new StateManager() + const addressBuffer = Buffer.from('a94f5374fce5edbc8e2a8697c15331677e6ebf0b', 'hex') + const getContractStorage = promisify((...args) => stateManager.getContractStorage(...args)) + try { + await getContractStorage(addressBuffer, Buffer.alloc(12)) + } catch (e) { + st.equal(e.message, 'Storage key must be 32 bytes long') + st.end() + return + } + + st.fail('Should have failed') + st.end() + }) }) tape('Original storage cache', async t => { @@ -191,7 +223,7 @@ tape('Original storage cache', async t => { const account = createAccount() await putAccount(address, account) - const key = Buffer.from('1234', 'hex') + const key = Buffer.from('1234567890123456789012345678901234567890123456789012345678901234', 'hex') const value = Buffer.from('1234', 'hex') t.test('should initially have empty storage value', async st => { @@ -232,7 +264,7 @@ tape('Original storage cache', async t => { }) t.test('should cache keys separately', async st => { - const key2 = Buffer.from('12', 'hex') + const key2 = Buffer.from('0000000000000000000000000000000000000000000000000000000000000012', 'hex') const value2 = Buffer.from('12', 'hex') const value3 = Buffer.from('123', 'hex') await putContractStorage(addressBuffer, key2, value2) @@ -257,4 +289,17 @@ tape('Original storage cache', async t => { st.end() }) + + t.test('getOriginalContractStorage should validate the key\'s length', async st => { + try { + await getOriginalContractStorage(addressBuffer, Buffer.alloc(12)) + } catch (e) { + st.equal(e.message, 'Storage key must be 32 bytes long') + st.end() + return + } + + st.fail('Should have failed') + st.end() + }) }) From 3bee52459dd86e606b77fd958c31ac23686a2cad Mon Sep 17 00:00:00 2001 From: Patricio Palladino Date: Thu, 25 Jul 2019 12:58:05 -0300 Subject: [PATCH 2/2] Remove LGTM warning --- lib/state/stateManager.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/state/stateManager.ts b/lib/state/stateManager.ts index bd05b61f01..97076ef9a9 100644 --- a/lib/state/stateManager.ts +++ b/lib/state/stateManager.ts @@ -1,5 +1,3 @@ -import { keccak256 } from 'ethereumjs-util' - const Set = require('core-js-pure/es/set') const Trie = require('merkle-patricia-tree/secure.js') const asyncLib = require('async')