[VM] StateManager: enforce 32-byte storage values when putting contra…

…ct storage

packages/vm/lib/state/stateManager.ts

@@ -1,6 +1,6 @@
 const Set = require('core-js-pure/es/set')
 import { SecureTrie as Trie } from 'merkle-patricia-tree'
-import { BN, toBuffer, keccak256, KECCAK256_NULL } from 'ethereumjs-util'
+import { BN, toBuffer, keccak256, KECCAK256_NULL, setLengthLeft, zeros } from 'ethereumjs-util'
 import { encode, decode } from 'rlp'
 import Common from '@ethereumjs/common'
 import { genesisStateByName } from '@ethereumjs/common/dist/genesisStates'
@@ -276,15 +276,21 @@ export default class DefaultStateManager implements StateManager {
    * corresponding to `address` at the provided `key`.
    * @param address -  Address to set a storage value for
    * @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 value - Value to set at `key` for account corresponding to `address`. Cannot be more than 32 bytes. Will be left-padded with zeros if less than 32 bytes. If it is a empty or filled with zeros, delete the value.
    */
   async putContractStorage(address: Buffer, key: Buffer, value: Buffer): Promise<void> {
     if (key.length !== 32) {
       throw new Error('Storage key must be 32 bytes long')
     }
 
+    if (value.length > 32) {
+      throw new Error('Storage key cannot be longer than 32 bytes')
+    } else if (value.length < 32) {
+      value = setLengthLeft(value, 32)
+    }
+
     await this._modifyContractStorage(address, async (storageTrie, done) => {
-      if (value && value.length) {
+      if (value && value.length && !value.equals(zeros(value.length))) {
         // format input
         const encodedValue = encode(value)
         await storageTrie.put(key, encodedValue)
@@ -415,7 +421,7 @@ export default class DefaultStateManager implements StateManager {
   }
 
   /**
-   * Dumps the the storage values for an `account` specified by `address`.
+   * Dumps the the RLP-encoded storage values for an `account` specified by `address`.
    * @param address - The address of the `account` to return storage for
    * @returns {Promise<StorageDump>} - The state of the account as an `Object` map.
    * Keys are are the storage keys, values are the storage values as strings.

packages/vm/tests/api/state/stateManager.js

@@ -1,5 +1,5 @@
 const tape = require('tape')
-const { toBuffer, keccak256, KECCAK256_RLP } = require('ethereumjs-util')
+const { toBuffer, keccak256, KECCAK256_RLP, zeros, setLengthLeft, rlp } = require('ethereumjs-util')
 const Common = require('@ethereumjs/common').default
 const Account = require('@ethereumjs/account').default
 const { DefaultStateManager } = require('../../../dist/state')
@@ -47,7 +47,7 @@ tape('StateManager', t => {
     // test contract storage cache
     await stateManager.checkpoint()
     const key = toBuffer('0x1234567890123456789012345678901234567890123456789012345678901234')
-    const value = Buffer.from('0x1234')
+    const value = setLengthLeft(Buffer.from('0x1234'), 32)
     await stateManager.putContractStorage(addressBuffer, key, value)
 
     const contract0 = await stateManager.getContractStorage(addressBuffer, key)
@@ -157,6 +157,7 @@ tape('StateManager', t => {
       st.skip('skip slow test when running in karma')
       return st.end()
     }
+
     const genesisData = require('ethereumjs-testing').getSingleFile(
       'BasicTests/genesishashestest.json',
     ) 
@@ -176,6 +177,7 @@ tape('StateManager', t => {
       st.skip('skip slow test when running in karma')
       return st.end()
     }
+
     const common = new Common({ chain: 'mainnet', hardfork: 'petersburg' })
     const expectedStateRoot = Buffer.from(common.genesis().stateRoot.slice(2), 'hex')
     const stateManager = new StateManager({ common: common })
@@ -217,11 +219,11 @@ tape('StateManager', t => {
     await stateManager.putAccount('a94f5374fce5edbc8e2a8697c15331677e6ebf0b', account)
 
     const key = toBuffer('0x1234567890123456789012345678901234567890123456789012345678901234')
-    const value = toBuffer('0x0a') // We used this value as its RLP encoding is also 0a
+    const value = toBuffer('0x0a') // note: this gets padded, the rlp-encoding of this value is a0....0a
     await stateManager.putContractStorage(addressBuffer, key, value)
 
     const data = await stateManager.dumpStorage(addressBuffer)
-    const expect = { [keccak256(key).toString('hex')]: '0a' }
+    const expect = { [keccak256(key).toString('hex')]: rlp.encode(setLengthLeft(value, 32)).toString('hex') }
     st.deepEqual(data, expect, 'should dump storage value')
 
     st.end()
@@ -300,25 +302,25 @@ tape('Original storage cache', async (t) => {
   t.test('should set original storage value', async (st) => {
     await stateManager.putContractStorage(addressBuffer, key, value)
     const res = await stateManager.getContractStorage(addressBuffer, key)
-    st.deepEqual(res, value)
+    st.deepEqual(res, setLengthLeft(value, 32))
 
     st.end()
   })
 
   t.test('should get original storage value', async (st) => {
     const res = await stateManager.getOriginalContractStorage(addressBuffer, key)
-    st.deepEqual(res, value)
+    st.deepEqual(res, setLengthLeft(value, 32))
     st.end()
   })
 
   t.test('should return correct original value after modification', async (st) => {
     const newValue = Buffer.from('1235', 'hex')
     await stateManager.putContractStorage(addressBuffer, key, newValue)
     const res = await stateManager.getContractStorage(addressBuffer, key)
-    st.deepEqual(res, newValue)
+    st.deepEqual(res, setLengthLeft(newValue, 32))
 
     const origRes = await stateManager.getOriginalContractStorage(addressBuffer, key)
-    st.deepEqual(origRes, value)
+    st.deepEqual(origRes, setLengthLeft(value, 32))
     st.end()
   })
 
@@ -332,22 +334,22 @@ tape('Original storage cache', async (t) => {
     await stateManager.putContractStorage(addressBuffer, key2, value2)
 
     let res = await stateManager.getContractStorage(addressBuffer, key2)
-    st.deepEqual(res, value2)
+    st.deepEqual(res, setLengthLeft(value2, 32))
     let origRes = await stateManager.getOriginalContractStorage(addressBuffer, key2)
-    st.deepEqual(origRes, value2)
+    st.deepEqual(origRes, setLengthLeft(value2, 32))
 
     await stateManager.putContractStorage(addressBuffer, key2, value3)
 
     res = await stateManager.getContractStorage(addressBuffer, key2)
-    st.deepEqual(res, value3)
+    st.deepEqual(res, setLengthLeft(value2, 32))
     origRes = await stateManager.getOriginalContractStorage(addressBuffer, key2)
-    st.deepEqual(origRes, value2)
+    st.deepEqual(origRes, setLengthLeft(value2, 32))
 
     // Check previous key
     res = await stateManager.getContractStorage(addressBuffer, key)
-    st.deepEqual(res, Buffer.from('1235', 'hex'))
+    st.deepEqual(res, setLengthLeft(Buffer.from('1235', 'hex'), 32))
     origRes = await stateManager.getOriginalContractStorage(addressBuffer, key)
-    st.deepEqual(origRes, value)
+    st.deepEqual(origRes, setLengthLeft(value, 32))
 
     st.end()
   })
@@ -420,3 +422,69 @@ tape('StateManager - Contract code', (tester) => {
     t.end()
   })
 })
+
+
+tape('StateManager - Contract storage', (tester) => {
+  const it = tester.test
+
+  it('should throw on storage values larger than 32 bytes', async(t) => {
+    t.plan(1)
+    const stateManager = new DefaultStateManager()
+    const address = zeros(20)
+    const key = zeros(32)
+    const value = Buffer.from(("aa").repeat(33), 'hex')
+    try {
+      await stateManager.putContractStorage(address, key, value)
+      t.fail("did not throw")
+    } catch (e) {
+      t.pass("threw on trying to set storage values larger than 32 bytes")
+    }
+    t.end()
+  })
+
+  it('should pad storage values lower than 32 bytes', async(t) => {
+    const stateManager = new DefaultStateManager()
+    const address = zeros(20)
+    const key0 = zeros(32)
+    const value0 = Buffer.from(("aa").repeat(31), 'hex') // put a value of 31-bytes length
+    const expect0 = setLengthLeft(value0, 32)
+    const key1 = Buffer.concat([zeros(31), Buffer.from('01', 'hex')])
+    const value1 = Buffer.from(("aa").repeat(1), 'hex') // put a vlaue of 1-byte length
+    const expect1 = setLengthLeft(value1, 32)
+
+    await stateManager.putContractStorage(address, key0, value0)
+    const slot0 = await stateManager.getContractStorage(address, key0)
+    t.ok(slot0.equals(expect0), 'value of 31 bytes padded correctly')
+
+    await stateManager.putContractStorage(address, key1, value1)
+    const slot1 = await stateManager.getContractStorage(address, key1)
+    t.ok(slot1.equals(expect1), 'value of 1 byte padded correctly')
+    t.end()
+  })
+
+  it ('should delete storage values which only consist of zero bytes', async(t) => {
+    const address = zeros(20)
+    const key = zeros(32)
+    const startValue = setLengthLeft(Buffer.from('01', 'hex'), 32)
+
+    const zeroLengths = [0, 1, 31, 32]  // checks for arbitrary-length zeros
+    t.plan(zeroLengths.length)
+
+    for (let length of zeroLengths) {
+      const stateManager = new DefaultStateManager()
+      const value = zeros(length)
+      await stateManager.putContractStorage(address, key, startValue)
+      const currentValue = await stateManager.getContractStorage(address, key)
+      if (!currentValue.equals(startValue)) {
+        // sanity check
+        t.fail("contract value not set correctly")
+      } else {
+        // delete the value 
+        await stateManager.putContractStorage(address, key, value)
+        const deleted = await stateManager.getContractStorage(address, key)
+        t.ok(deleted.equals(zeros(0)), "the storage key should be deleted")
+      }
+    }
+    t.end()
+  })
+})