From e6a21011800157828305a881d3604aa47b128558 Mon Sep 17 00:00:00 2001 From: mirazu Date: Thu, 30 Jun 2022 21:23:28 +0900 Subject: [PATCH 1/5] fix: slot-overwrite bug --- package.json | 4 +- src/factories/ethers-interface.ts | 51 +++++-------------- src/logic/editable-storage-logic.ts | 18 ++++++- src/utils/storage.ts | 21 ++++++++ test/contracts/mock/Bug.sol | 7 +++ .../watchable-function-logic/Storage.sol | 26 ---------- .../StorageDuplicate.sol | 26 ---------- test/unit/fake/initialization.spec.ts | 20 +------- test/unit/mock/editable-storage-logic.spec.ts | 3 +- test/unit/mock/mytest.spec.ts | 26 ++++++++++ test/unit/mock/mytest.spec2.ts | 25 +++++++++ yarn.lock | 14 ----- 12 files changed, 111 insertions(+), 130 deletions(-) create mode 100644 test/contracts/mock/Bug.sol delete mode 100644 test/contracts/watchable-function-logic/Storage.sol delete mode 100644 test/contracts/watchable-function-logic/StorageDuplicate.sol create mode 100644 test/unit/mock/mytest.spec.ts create mode 100644 test/unit/mock/mytest.spec2.ts diff --git a/package.json b/package.json index 1452bf7..4361a09 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,9 @@ "build": "rm -rf dist && tsc -p tsconfig.build.json", "compile": "hardhat compile", "test:nocompile": "cross-env mocha 'test/unit/**/*.spec.ts'", + "mytest:nocompile": "cross-env mocha 'test/unit/mock/mytest.spec.ts'", "test": "yarn compile && yarn test:nocompile", + "mytest": "yarn compile && yarn mytest:nocompile", "lint:check": "cross-env solhint 'contracts/**/*.sol' 'interfaces/**/*.sol' && cross-env prettier --check './**'", "lint:fix": "sort-package-json && cross-env prettier --write './**' && cross-env solhint --fix 'contracts/**/*.sol' 'interfaces/**/*.sol'", "docs:md": "pandoc README.rst -f rst -t markdown -o README.md", @@ -74,7 +76,6 @@ "@typechain/ethers-v5": "7.0.1", "@typechain/hardhat": "2.0.2", "@types/chai": "4.2.18", - "@types/chai-as-promised": "7.1.5", "@types/diff": "^5.0.1", "@types/lodash": "4.14.170", "@types/lodash.isequal": "^4.5.5", @@ -83,7 +84,6 @@ "@types/node": "15.12.2", "@types/semver": "^7.3.8", "chai": "4.3.4", - "chai-as-promised": "7.1.1", "cross-env": "7.0.3", "ethereum-waffle": "3.4.0", "ethers": "5.4.1", diff --git a/src/factories/ethers-interface.ts b/src/factories/ethers-interface.ts index 5dcfa2c..abb387a 100644 --- a/src/factories/ethers-interface.ts +++ b/src/factories/ethers-interface.ts @@ -5,14 +5,18 @@ import { FakeContractSpec } from '../types'; export async function ethersInterfaceFromSpec(spec: FakeContractSpec): Promise { if (typeof spec === 'string') { try { - if (isMaybeJsonObject(spec)) { - return await ethersInterfaceFromAbi(spec); - } else { - return await ethersInterfaceFromContractName(spec); - } - } catch (err) { - throw err; - } + return new ethers.utils.Interface(spec); + } catch {} + + try { + return (await (hre as any).ethers.getContractFactory(spec)).interface; + } catch {} + + try { + return (await (hre as any).ethers.getContractAt(spec, ethers.constants.AddressZero)).interface; + } catch {} + + throw new Error(`unable to generate smock spec from string`); } let foundInterface: any = spec; @@ -28,34 +32,3 @@ export async function ethersInterfaceFromSpec(spec: FakeContractSpec): Promise { - try { - return new ethers.utils.Interface(abi); - } catch (err) { - const error: Error = err as Error; - throw new Error(`unable to generate smock spec from abi string.\n${error.message}`); - } -} - -async function ethersInterfaceFromContractName(contractNameOrFullyQualifiedName: string): Promise { - let error: Error | null = null; - try { - return (await (hre as any).ethers.getContractFactory(contractNameOrFullyQualifiedName)).interface; - } catch (err) { - error = err as Error; - } - - try { - return (await (hre as any).ethers.getContractAt(contractNameOrFullyQualifiedName, ethers.constants.AddressZero)).interface; - } catch (err) { - error = err as Error; - } - - throw new Error(`unable to generate smock spec from contract name.\n${error.message}`); -} - -function isMaybeJsonObject(str: string): boolean { - let strJson = str.trim(); - return strJson.charAt(0) == '{' && strJson.charAt(strJson.length - 1) == '}'; -} diff --git a/src/logic/editable-storage-logic.ts b/src/logic/editable-storage-logic.ts index e21f202..cd8af47 100644 --- a/src/logic/editable-storage-logic.ts +++ b/src/logic/editable-storage-logic.ts @@ -1,5 +1,5 @@ import { SetVariablesType, SmockVMManager } from '../types'; -import { fromHexString, toFancyAddress } from '../utils'; +import { fromHexString, remove0x, toFancyAddress, toHexString } from '../utils'; import { computeStorageSlots, SolidityStorageLayout } from '../utils/storage'; export class EditableStorageLogic { @@ -18,8 +18,22 @@ export class EditableStorageLogic { const slots = computeStorageSlots(this.storageLayout, { [variableName]: value }); + // for (const slot of slots) { + // await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val)); + // } + for (const slot of slots) { - await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val)); + let prevStorageValue = await this.vmManager.getContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key)); + let stringValue = remove0x(toHexString(prevStorageValue)); + if (stringValue && (slot.type === 'address' || slot.type === 'bool' || slot.type.startsWith('contract'))) { + // console.log('_______________________________', stringValue, slot.val); + let lastResult = slot.val.slice(0, slot.val.length - stringValue.length).concat(stringValue); + await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(lastResult)); + // console.log('>>>>>>>>>>>>>>>>>>>>>>>', slot.key, lastResult); + } else { + await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val)); + // console.log('>>>>>>>>>>>>>>>>>>>>>>>', slot.key, slot.val); + } } } diff --git a/src/utils/storage.ts b/src/utils/storage.ts index 489f61e..4eb1f44 100644 --- a/src/utils/storage.ts +++ b/src/utils/storage.ts @@ -40,6 +40,7 @@ export interface SolidityStorageLayout { interface StorageSlotPair { key: string; val: string; + type: string; } /** @@ -67,6 +68,13 @@ export async function getStorageLayout(name: string): Promise { return entry.label === variableName; }); @@ -91,10 +100,14 @@ export function computeStorageSlots(storageLayout: SolidityStorageLayout, variab throw new Error(`Variable name not found in storage layout: ${variableName}`); } + // console.log('___ storageObj ___', storageObj); + // console.log('___ slots before ___', slots); // Encode this variable as series of storage slot key/value pairs and save it. slots = slots.concat(encodeVariable(variableValue, storageObj, storageLayout.types)); + // console.log('___ slots after ___', slots); } + // console.log('___ 1 ___', slots); // Dealing with packed storage slots now. We know that a storage slot is packed when two storage // slots produced by the above encoding have the same key. In this case, we want to merge the two // values into a single bytes32 value. We'll throw an error if the two values overlap (have some @@ -139,6 +152,7 @@ export function computeStorageSlots(storageLayout: SolidityStorageLayout, variab prevSlots.push({ key: slot.key, val: mergedVal, + type: slot.type, }); } @@ -228,6 +242,7 @@ function encodeVariable( { key: slotKey, val: padNumHexSlotValue(variable, storageObj.offset), + type: variableType.label, }, ]; } else if (variableType.label === 'bool') { @@ -249,6 +264,7 @@ function encodeVariable( { key: slotKey, val: padNumHexSlotValue(variable ? '1' : '0', storageObj.offset), + type: variableType.label, }, ]; } else if (variableType.label.startsWith('bytes')) { @@ -260,6 +276,7 @@ function encodeVariable( { key: slotKey, val: padBytesHexSlotValue(remove0x(variable).padEnd(parseInt(variableType.numberOfBytes, 10) * 2, '0'), storageObj.offset), + type: variableType.label, }, ]; } else if (variableType.label.startsWith('uint') || variableType.label.startsWith('int')) { @@ -271,6 +288,7 @@ function encodeVariable( { key: slotKey, val: padNumHexSlotValue(variable, storageObj.offset), + type: variableType.label, }, ]; } else if (variableType.label.startsWith('struct')) { @@ -314,6 +332,7 @@ function encodeVariable( ethers.BigNumber.from(bytes.length * 2).toHexString(), ]) ), + type: variableType.label, }, ]; } else { @@ -324,6 +343,7 @@ function encodeVariable( slots = slots.concat({ key: slotKey, val: padNumHexSlotValue(bytes.length * 2 + 1, 0), + type: variableType.label, }); // Each storage slot has 32 bytes so we make sure to slice the large bytes into 32bytes chunks @@ -335,6 +355,7 @@ function encodeVariable( slots = slots.concat({ key: key, val: ethers.utils.hexlify(ethers.utils.concat([bytes.slice(i * 32, i * 32 + 32), ethers.constants.HashZero]).slice(0, 32)), + type: variableType.label, }); } diff --git a/test/contracts/mock/Bug.sol b/test/contracts/mock/Bug.sol new file mode 100644 index 0000000..ec66c6d --- /dev/null +++ b/test/contracts/mock/Bug.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +contract Bug { + address public myAddress; + bool public myBool; +} diff --git a/test/contracts/watchable-function-logic/Storage.sol b/test/contracts/watchable-function-logic/Storage.sol deleted file mode 100644 index 7b87578..0000000 --- a/test/contracts/watchable-function-logic/Storage.sol +++ /dev/null @@ -1,26 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.4; - -/** - * @title Storage - * @dev Store & retrieve value in a variable - */ -contract Storage { - uint256 number; - - /** - * @dev Store value in variable - * @param num value to store - */ - function store(uint256 num) public { - number = num; - } - - /** - * @dev Return value - * @return value of 'number' - */ - function retrieve() public view returns (uint256) { - return number; - } -} diff --git a/test/contracts/watchable-function-logic/StorageDuplicate.sol b/test/contracts/watchable-function-logic/StorageDuplicate.sol deleted file mode 100644 index 7b87578..0000000 --- a/test/contracts/watchable-function-logic/StorageDuplicate.sol +++ /dev/null @@ -1,26 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.4; - -/** - * @title Storage - * @dev Store & retrieve value in a variable - */ -contract Storage { - uint256 number; - - /** - * @dev Store value in variable - * @param num value to store - */ - function store(uint256 num) public { - number = num; - } - - /** - * @dev Return value - * @return value of 'number' - */ - function retrieve() public view returns (uint256) { - return number; - } -} diff --git a/test/unit/fake/initialization.spec.ts b/test/unit/fake/initialization.spec.ts index 3b0740b..8779c62 100644 --- a/test/unit/fake/initialization.spec.ts +++ b/test/unit/fake/initialization.spec.ts @@ -3,11 +3,10 @@ import { makeRandomAddress } from '@src/utils'; import { Receiver, Receiver__factory, Returner } from '@typechained'; import receiverArtifact from 'artifacts/test/contracts/watchable-function-logic/Receiver.sol/Receiver.json'; import chai, { expect } from 'chai'; -import chaiAsPromised from 'chai-as-promised'; import { ethers, network } from 'hardhat'; import storageArtifact from 'test/unit/fake/testdata/Storage.json'; -chai.use(chaiAsPromised); +chai.use(smock.matchers); describe('Fake: Initialization', () => { it('should work with the contract name', async () => { @@ -91,21 +90,4 @@ describe('Fake: Initialization', () => { const fake = await smock.fake(storageArtifact); expect(fake.store._watchable).not.to.be.undefined; }); - - it('should throw error for invalid json string abi', async () => { - await expect(smock.fake(`{invalid}`)).to.be.rejectedWith( - Error, - `unable to generate smock spec from abi string.\nUnexpected token i in JSON at position 1` - ); - }); - - it('should throw error for non existent contract', async () => { - let regex: RegExp = /unable to generate smock spec from contract name*/; - await expect(smock.fake('NonExistentContract')).to.be.rejectedWith(Error, regex); - }); - - it('should throw error if contract name is ambiguous', async () => { - let regex: RegExp = /unable to generate smock spec from contract name*/; - await expect(smock.fake('Storage')).to.be.rejectedWith(Error, regex); - }); }); diff --git a/test/unit/mock/editable-storage-logic.spec.ts b/test/unit/mock/editable-storage-logic.spec.ts index b6d16ae..a74576e 100644 --- a/test/unit/mock/editable-storage-logic.spec.ts +++ b/test/unit/mock/editable-storage-logic.spec.ts @@ -14,7 +14,7 @@ describe('Mock: Editable storage logic', () => { }); beforeEach(async () => { - mock = await storageGetterFactory.deploy(1); + mock = await storageGetterFactory.deploy(4); }); describe('setVariable', () => { @@ -64,7 +64,6 @@ describe('Mock: Editable storage logic', () => { it('should be able to set an address in a packed storage slot', async () => { await mock.setVariable('_packedB', ADDRESS_EXAMPLE); - expect(await mock.getPackedAddress()).to.equal(ADDRESS_EXAMPLE); }); diff --git a/test/unit/mock/mytest.spec.ts b/test/unit/mock/mytest.spec.ts new file mode 100644 index 0000000..41223ad --- /dev/null +++ b/test/unit/mock/mytest.spec.ts @@ -0,0 +1,26 @@ +import { MockContract, MockContractFactory, smock } from '@src'; +import { ADDRESS_EXAMPLE } from '@test-utils'; +import { Bug, Bug__factory } from '@typechained'; +import { expect } from 'chai'; + +describe('Bug Fix', () => { + let bugFactory: MockContractFactory; + let mock: MockContract; + + before(async () => { + bugFactory = await smock.mock('Bug'); + }); + + beforeEach(async () => { + mock = await bugFactory.deploy(); + }); + + describe('setVariable', () => { + it('should be able to fix slot overwrite on packed variables', async () => { + await mock.setVariable('myAddress', ADDRESS_EXAMPLE); + await mock.setVariable('myBool', true); + console.log(await mock.myAddress()); + expect(await mock.myAddress()).to.equal(ADDRESS_EXAMPLE); + }); + }); +}); diff --git a/test/unit/mock/mytest.spec2.ts b/test/unit/mock/mytest.spec2.ts new file mode 100644 index 0000000..4406b0d --- /dev/null +++ b/test/unit/mock/mytest.spec2.ts @@ -0,0 +1,25 @@ +import { MockContract, MockContractFactory, smock } from '@src'; +import { ADDRESS_EXAMPLE } from '@test-utils'; +import { StorageGetter, StorageGetter__factory } from '@typechained'; +import { expect } from 'chai'; + +describe('Mock: Editable storage logic', () => { + let storageGetterFactory: MockContractFactory; + let mock: MockContract; + + before(async () => { + storageGetterFactory = await smock.mock('StorageGetter'); + }); + + beforeEach(async () => { + mock = await storageGetterFactory.deploy(1); + }); + + describe('setVariable', () => { + it('should be able to fix bug', async () => { + await mock.setVariable('_address', ADDRESS_EXAMPLE); + await mock.setVariable('_bool', true); + expect(await mock.getAddress()).to.equal(ADDRESS_EXAMPLE); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index bdd33cc..d9919f5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -914,13 +914,6 @@ dependencies: "@types/node" "*" -"@types/chai-as-promised@7.1.5": - version "7.1.5" - resolved "https://registry.yarnpkg.com/@types/chai-as-promised/-/chai-as-promised-7.1.5.tgz#6e016811f6c7a64f2eed823191c3a6955094e255" - integrity sha512-jStwss93SITGBwt/niYrkf2C+/1KTeZCZl1LaeezTlqppAKeoQC7jxyqYuP72sxBGKCIbw7oHgbYssIRzT5FCQ== - dependencies: - "@types/chai" "*" - "@types/chai@*": version "4.2.21" resolved "https://registry.yarnpkg.com/@types/chai/-/chai-4.2.21.tgz#9f35a5643129df132cf3b5c1ec64046ea1af0650" @@ -2424,13 +2417,6 @@ caseless@~0.12.0: resolved "https://registry.yarnpkg.com/caseless/-/caseless-0.12.0.tgz#1b681c21ff84033c826543090689420d187151dc" integrity sha1-G2gcIf+EAzyCZUMJBolCDRhxUdw= -chai-as-promised@7.1.1: - version "7.1.1" - resolved "https://registry.yarnpkg.com/chai-as-promised/-/chai-as-promised-7.1.1.tgz#08645d825deb8696ee61725dbf590c012eb00ca0" - integrity sha512-azL6xMoi+uxu6z4rhWQ1jbdUhOMhis2PvscD/xjLqNMkv3BPPp2JyyuTHOrf9BOosGpNQ11v6BKv/g57RXbiaA== - dependencies: - check-error "^1.0.2" - chai@4.3.4: version "4.3.4" resolved "https://registry.yarnpkg.com/chai/-/chai-4.3.4.tgz#b55e655b31e1eac7099be4c08c21964fce2e6c49" From e53b89bfb92962f06c15e08c4ea17d8c6c5f5be8 Mon Sep 17 00:00:00 2001 From: mirazu Date: Fri, 1 Jul 2022 19:07:14 +0900 Subject: [PATCH 2/5] fix: slot-overwrite bug --- package.json | 3 +- src/factories/ethers-interface.ts | 51 ++++++++++++++++++++++------- src/logic/editable-storage-logic.ts | 7 ---- src/utils/storage.ts | 12 ------- test/contracts/mock/Bug.sol | 4 +++ test/unit/mock/mytest.spec.ts | 18 ++++++++-- test/utils/constants.ts | 4 +++ yarn.lock | 7 ++++ 8 files changed, 71 insertions(+), 35 deletions(-) diff --git a/package.json b/package.json index 4361a09..9032790 100644 --- a/package.json +++ b/package.json @@ -36,9 +36,7 @@ "build": "rm -rf dist && tsc -p tsconfig.build.json", "compile": "hardhat compile", "test:nocompile": "cross-env mocha 'test/unit/**/*.spec.ts'", - "mytest:nocompile": "cross-env mocha 'test/unit/mock/mytest.spec.ts'", "test": "yarn compile && yarn test:nocompile", - "mytest": "yarn compile && yarn mytest:nocompile", "lint:check": "cross-env solhint 'contracts/**/*.sol' 'interfaces/**/*.sol' && cross-env prettier --check './**'", "lint:fix": "sort-package-json && cross-env prettier --write './**' && cross-env solhint --fix 'contracts/**/*.sol' 'interfaces/**/*.sol'", "docs:md": "pandoc README.rst -f rst -t markdown -o README.md", @@ -84,6 +82,7 @@ "@types/node": "15.12.2", "@types/semver": "^7.3.8", "chai": "4.3.4", + "chai-as-promised": "7.1.1", "cross-env": "7.0.3", "ethereum-waffle": "3.4.0", "ethers": "5.4.1", diff --git a/src/factories/ethers-interface.ts b/src/factories/ethers-interface.ts index abb387a..5dcfa2c 100644 --- a/src/factories/ethers-interface.ts +++ b/src/factories/ethers-interface.ts @@ -5,18 +5,14 @@ import { FakeContractSpec } from '../types'; export async function ethersInterfaceFromSpec(spec: FakeContractSpec): Promise { if (typeof spec === 'string') { try { - return new ethers.utils.Interface(spec); - } catch {} - - try { - return (await (hre as any).ethers.getContractFactory(spec)).interface; - } catch {} - - try { - return (await (hre as any).ethers.getContractAt(spec, ethers.constants.AddressZero)).interface; - } catch {} - - throw new Error(`unable to generate smock spec from string`); + if (isMaybeJsonObject(spec)) { + return await ethersInterfaceFromAbi(spec); + } else { + return await ethersInterfaceFromContractName(spec); + } + } catch (err) { + throw err; + } } let foundInterface: any = spec; @@ -32,3 +28,34 @@ export async function ethersInterfaceFromSpec(spec: FakeContractSpec): Promise { + try { + return new ethers.utils.Interface(abi); + } catch (err) { + const error: Error = err as Error; + throw new Error(`unable to generate smock spec from abi string.\n${error.message}`); + } +} + +async function ethersInterfaceFromContractName(contractNameOrFullyQualifiedName: string): Promise { + let error: Error | null = null; + try { + return (await (hre as any).ethers.getContractFactory(contractNameOrFullyQualifiedName)).interface; + } catch (err) { + error = err as Error; + } + + try { + return (await (hre as any).ethers.getContractAt(contractNameOrFullyQualifiedName, ethers.constants.AddressZero)).interface; + } catch (err) { + error = err as Error; + } + + throw new Error(`unable to generate smock spec from contract name.\n${error.message}`); +} + +function isMaybeJsonObject(str: string): boolean { + let strJson = str.trim(); + return strJson.charAt(0) == '{' && strJson.charAt(strJson.length - 1) == '}'; +} diff --git a/src/logic/editable-storage-logic.ts b/src/logic/editable-storage-logic.ts index cd8af47..6922078 100644 --- a/src/logic/editable-storage-logic.ts +++ b/src/logic/editable-storage-logic.ts @@ -18,21 +18,14 @@ export class EditableStorageLogic { const slots = computeStorageSlots(this.storageLayout, { [variableName]: value }); - // for (const slot of slots) { - // await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val)); - // } - for (const slot of slots) { let prevStorageValue = await this.vmManager.getContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key)); let stringValue = remove0x(toHexString(prevStorageValue)); if (stringValue && (slot.type === 'address' || slot.type === 'bool' || slot.type.startsWith('contract'))) { - // console.log('_______________________________', stringValue, slot.val); let lastResult = slot.val.slice(0, slot.val.length - stringValue.length).concat(stringValue); await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(lastResult)); - // console.log('>>>>>>>>>>>>>>>>>>>>>>>', slot.key, lastResult); } else { await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val)); - // console.log('>>>>>>>>>>>>>>>>>>>>>>>', slot.key, slot.val); } } } diff --git a/src/utils/storage.ts b/src/utils/storage.ts index 4eb1f44..b322035 100644 --- a/src/utils/storage.ts +++ b/src/utils/storage.ts @@ -68,13 +68,6 @@ export async function getStorageLayout(name: string): Promise { return entry.label === variableName; }); @@ -100,14 +92,10 @@ export function computeStorageSlots(storageLayout: SolidityStorageLayout, variab throw new Error(`Variable name not found in storage layout: ${variableName}`); } - // console.log('___ storageObj ___', storageObj); - // console.log('___ slots before ___', slots); // Encode this variable as series of storage slot key/value pairs and save it. slots = slots.concat(encodeVariable(variableValue, storageObj, storageLayout.types)); - // console.log('___ slots after ___', slots); } - // console.log('___ 1 ___', slots); // Dealing with packed storage slots now. We know that a storage slot is packed when two storage // slots produced by the above encoding have the same key. In this case, we want to merge the two // values into a single bytes32 value. We'll throw an error if the two values overlap (have some diff --git a/test/contracts/mock/Bug.sol b/test/contracts/mock/Bug.sol index ec66c6d..193eb91 100644 --- a/test/contracts/mock/Bug.sol +++ b/test/contracts/mock/Bug.sol @@ -4,4 +4,8 @@ pragma solidity ^0.8.4; contract Bug { address public myAddress; bool public myBool; + address public newAddress; + address public newAddress2; + bool public newBool; + bool public newBool2; } diff --git a/test/unit/mock/mytest.spec.ts b/test/unit/mock/mytest.spec.ts index 41223ad..519da63 100644 --- a/test/unit/mock/mytest.spec.ts +++ b/test/unit/mock/mytest.spec.ts @@ -1,5 +1,5 @@ import { MockContract, MockContractFactory, smock } from '@src'; -import { ADDRESS_EXAMPLE } from '@test-utils'; +import { ADDRESS_EXAMPLE, ADDRESS_EXAMPLE2, ADDRESS_EXAMPLE3 } from '@test-utils'; import { Bug, Bug__factory } from '@typechained'; import { expect } from 'chai'; @@ -19,8 +19,22 @@ describe('Bug Fix', () => { it('should be able to fix slot overwrite on packed variables', async () => { await mock.setVariable('myAddress', ADDRESS_EXAMPLE); await mock.setVariable('myBool', true); - console.log(await mock.myAddress()); + await mock.setVariable('newAddress', ADDRESS_EXAMPLE2); + await mock.setVariable('newAddress2', ADDRESS_EXAMPLE3); + await mock.setVariable('newBool', false); + await mock.setVariable('newBool2', true); + console.log('____myAddress____', await mock.myAddress()); + console.log('____myBool____', await mock.myBool()); + console.log('____newAddress____', await mock.newAddress()); + console.log('____newAddress2____', await mock.newAddress2()); + console.log('____newBool____', await mock.newBool()); + console.log('____newBool2____', await mock.newBool2()); expect(await mock.myAddress()).to.equal(ADDRESS_EXAMPLE); + expect(await mock.myBool()).to.equal(true); + expect(await mock.newAddress()).to.equal(ADDRESS_EXAMPLE2); + expect(await mock.newAddress2()).to.equal(ADDRESS_EXAMPLE3); + expect(await mock.newBool()).to.equal(false); + expect(await mock.newBool2()).to.equal(true); }); }); }); diff --git a/test/utils/constants.ts b/test/utils/constants.ts index cf12ef5..d1363bf 100644 --- a/test/utils/constants.ts +++ b/test/utils/constants.ts @@ -2,6 +2,10 @@ import { utils } from 'ethers'; export const ADDRESS_EXAMPLE = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'; +export const ADDRESS_EXAMPLE2 = '0x4fd96172C40913c8100a47337f21DBf25C9e2fC6'; + +export const ADDRESS_EXAMPLE3 = '0x62eA762d9f2672f82d26b94f56849881831B9eaB'; + export const BYTES32_EXAMPLE = '0x1234123412341234123412341234123412341234123412341234123412341234'; export const BYTES_EXAMPLE = diff --git a/yarn.lock b/yarn.lock index d9919f5..efc8350 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2417,6 +2417,13 @@ caseless@~0.12.0: resolved "https://registry.yarnpkg.com/caseless/-/caseless-0.12.0.tgz#1b681c21ff84033c826543090689420d187151dc" integrity sha1-G2gcIf+EAzyCZUMJBolCDRhxUdw= +chai-as-promised@7.1.1: + version "7.1.1" + resolved "https://registry.yarnpkg.com/chai-as-promised/-/chai-as-promised-7.1.1.tgz#08645d825deb8696ee61725dbf590c012eb00ca0" + integrity sha512-azL6xMoi+uxu6z4rhWQ1jbdUhOMhis2PvscD/xjLqNMkv3BPPp2JyyuTHOrf9BOosGpNQ11v6BKv/g57RXbiaA== + dependencies: + check-error "^1.0.2" + chai@4.3.4: version "4.3.4" resolved "https://registry.yarnpkg.com/chai/-/chai-4.3.4.tgz#b55e655b31e1eac7099be4c08c21964fce2e6c49" From 2b470511ab9b0074bfd8fd149dc89c0da75a5374 Mon Sep 17 00:00:00 2001 From: mirazu Date: Sat, 2 Jul 2022 01:59:35 +0900 Subject: [PATCH 3/5] fix: slot-overwrite bug --- package.json | 1 + .../mock/{Bug.sol => SlotOverwrite.sol} | 2 +- test/unit/mock/mytest.spec2.ts | 25 ------------------- ...{mytest.spec.ts => slot-overwrite.spec.ts} | 10 ++------ 4 files changed, 4 insertions(+), 34 deletions(-) rename test/contracts/mock/{Bug.sol => SlotOverwrite.sol} (89%) delete mode 100644 test/unit/mock/mytest.spec2.ts rename test/unit/mock/{mytest.spec.ts => slot-overwrite.spec.ts} (70%) diff --git a/package.json b/package.json index 9032790..1452bf7 100644 --- a/package.json +++ b/package.json @@ -74,6 +74,7 @@ "@typechain/ethers-v5": "7.0.1", "@typechain/hardhat": "2.0.2", "@types/chai": "4.2.18", + "@types/chai-as-promised": "7.1.5", "@types/diff": "^5.0.1", "@types/lodash": "4.14.170", "@types/lodash.isequal": "^4.5.5", diff --git a/test/contracts/mock/Bug.sol b/test/contracts/mock/SlotOverwrite.sol similarity index 89% rename from test/contracts/mock/Bug.sol rename to test/contracts/mock/SlotOverwrite.sol index 193eb91..d7bd236 100644 --- a/test/contracts/mock/Bug.sol +++ b/test/contracts/mock/SlotOverwrite.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.4; -contract Bug { +contract SlotOverwrite { address public myAddress; bool public myBool; address public newAddress; diff --git a/test/unit/mock/mytest.spec2.ts b/test/unit/mock/mytest.spec2.ts deleted file mode 100644 index 4406b0d..0000000 --- a/test/unit/mock/mytest.spec2.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { MockContract, MockContractFactory, smock } from '@src'; -import { ADDRESS_EXAMPLE } from '@test-utils'; -import { StorageGetter, StorageGetter__factory } from '@typechained'; -import { expect } from 'chai'; - -describe('Mock: Editable storage logic', () => { - let storageGetterFactory: MockContractFactory; - let mock: MockContract; - - before(async () => { - storageGetterFactory = await smock.mock('StorageGetter'); - }); - - beforeEach(async () => { - mock = await storageGetterFactory.deploy(1); - }); - - describe('setVariable', () => { - it('should be able to fix bug', async () => { - await mock.setVariable('_address', ADDRESS_EXAMPLE); - await mock.setVariable('_bool', true); - expect(await mock.getAddress()).to.equal(ADDRESS_EXAMPLE); - }); - }); -}); diff --git a/test/unit/mock/mytest.spec.ts b/test/unit/mock/slot-overwrite.spec.ts similarity index 70% rename from test/unit/mock/mytest.spec.ts rename to test/unit/mock/slot-overwrite.spec.ts index 519da63..d5b9c03 100644 --- a/test/unit/mock/mytest.spec.ts +++ b/test/unit/mock/slot-overwrite.spec.ts @@ -3,7 +3,7 @@ import { ADDRESS_EXAMPLE, ADDRESS_EXAMPLE2, ADDRESS_EXAMPLE3 } from '@test-utils import { Bug, Bug__factory } from '@typechained'; import { expect } from 'chai'; -describe('Bug Fix', () => { +describe('Mock: Slot Overwrite', () => { let bugFactory: MockContractFactory; let mock: MockContract; @@ -16,19 +16,13 @@ describe('Bug Fix', () => { }); describe('setVariable', () => { - it('should be able to fix slot overwrite on packed variables', async () => { + it('should not be able to overwrite slot', async () => { await mock.setVariable('myAddress', ADDRESS_EXAMPLE); await mock.setVariable('myBool', true); await mock.setVariable('newAddress', ADDRESS_EXAMPLE2); await mock.setVariable('newAddress2', ADDRESS_EXAMPLE3); await mock.setVariable('newBool', false); await mock.setVariable('newBool2', true); - console.log('____myAddress____', await mock.myAddress()); - console.log('____myBool____', await mock.myBool()); - console.log('____newAddress____', await mock.newAddress()); - console.log('____newAddress2____', await mock.newAddress2()); - console.log('____newBool____', await mock.newBool()); - console.log('____newBool2____', await mock.newBool2()); expect(await mock.myAddress()).to.equal(ADDRESS_EXAMPLE); expect(await mock.myBool()).to.equal(true); expect(await mock.newAddress()).to.equal(ADDRESS_EXAMPLE2); From 7f7fdff101944bdf101a680636f19dac1c3bdc97 Mon Sep 17 00:00:00 2001 From: mirazu Date: Tue, 5 Jul 2022 23:27:46 +0900 Subject: [PATCH 4/5] fix: slot-overwrite bug --- test/contracts/mock/SlotOverwrite.sol | 11 ------ test/contracts/mock/StorageGetter.sol | 4 +++ .../watchable-function-logic/Storage.sol | 26 ++++++++++++++ .../StorageDuplicate.sol | 26 ++++++++++++++ test/unit/fake/initialization.spec.ts | 20 ++++++++++- test/unit/mock/editable-storage-logic.spec.ts | 11 +++++- test/unit/mock/slot-overwrite.spec.ts | 34 ------------------- test/utils/constants.ts | 4 --- yarn.lock | 7 ++++ 9 files changed, 92 insertions(+), 51 deletions(-) delete mode 100644 test/contracts/mock/SlotOverwrite.sol create mode 100644 test/contracts/watchable-function-logic/Storage.sol create mode 100644 test/contracts/watchable-function-logic/StorageDuplicate.sol delete mode 100644 test/unit/mock/slot-overwrite.spec.ts diff --git a/test/contracts/mock/SlotOverwrite.sol b/test/contracts/mock/SlotOverwrite.sol deleted file mode 100644 index d7bd236..0000000 --- a/test/contracts/mock/SlotOverwrite.sol +++ /dev/null @@ -1,11 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.4; - -contract SlotOverwrite { - address public myAddress; - bool public myBool; - address public newAddress; - address public newAddress2; - bool public newBool; - bool public newBool2; -} diff --git a/test/contracts/mock/StorageGetter.sol b/test/contracts/mock/StorageGetter.sol index 4e0b148..c547ec9 100644 --- a/test/contracts/mock/StorageGetter.sol +++ b/test/contracts/mock/StorageGetter.sol @@ -33,6 +33,10 @@ contract StorageGetter { bool internal _packedA; address internal _packedB; + // Testing slot-overwrite + address public _slotA; + bool public _slotB; + constructor(uint256 _inA) { _constructorUint256 = _inA; } diff --git a/test/contracts/watchable-function-logic/Storage.sol b/test/contracts/watchable-function-logic/Storage.sol new file mode 100644 index 0000000..7b87578 --- /dev/null +++ b/test/contracts/watchable-function-logic/Storage.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +/** + * @title Storage + * @dev Store & retrieve value in a variable + */ +contract Storage { + uint256 number; + + /** + * @dev Store value in variable + * @param num value to store + */ + function store(uint256 num) public { + number = num; + } + + /** + * @dev Return value + * @return value of 'number' + */ + function retrieve() public view returns (uint256) { + return number; + } +} diff --git a/test/contracts/watchable-function-logic/StorageDuplicate.sol b/test/contracts/watchable-function-logic/StorageDuplicate.sol new file mode 100644 index 0000000..7b87578 --- /dev/null +++ b/test/contracts/watchable-function-logic/StorageDuplicate.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +/** + * @title Storage + * @dev Store & retrieve value in a variable + */ +contract Storage { + uint256 number; + + /** + * @dev Store value in variable + * @param num value to store + */ + function store(uint256 num) public { + number = num; + } + + /** + * @dev Return value + * @return value of 'number' + */ + function retrieve() public view returns (uint256) { + return number; + } +} diff --git a/test/unit/fake/initialization.spec.ts b/test/unit/fake/initialization.spec.ts index 8779c62..3b0740b 100644 --- a/test/unit/fake/initialization.spec.ts +++ b/test/unit/fake/initialization.spec.ts @@ -3,10 +3,11 @@ import { makeRandomAddress } from '@src/utils'; import { Receiver, Receiver__factory, Returner } from '@typechained'; import receiverArtifact from 'artifacts/test/contracts/watchable-function-logic/Receiver.sol/Receiver.json'; import chai, { expect } from 'chai'; +import chaiAsPromised from 'chai-as-promised'; import { ethers, network } from 'hardhat'; import storageArtifact from 'test/unit/fake/testdata/Storage.json'; -chai.use(smock.matchers); +chai.use(chaiAsPromised); describe('Fake: Initialization', () => { it('should work with the contract name', async () => { @@ -90,4 +91,21 @@ describe('Fake: Initialization', () => { const fake = await smock.fake(storageArtifact); expect(fake.store._watchable).not.to.be.undefined; }); + + it('should throw error for invalid json string abi', async () => { + await expect(smock.fake(`{invalid}`)).to.be.rejectedWith( + Error, + `unable to generate smock spec from abi string.\nUnexpected token i in JSON at position 1` + ); + }); + + it('should throw error for non existent contract', async () => { + let regex: RegExp = /unable to generate smock spec from contract name*/; + await expect(smock.fake('NonExistentContract')).to.be.rejectedWith(Error, regex); + }); + + it('should throw error if contract name is ambiguous', async () => { + let regex: RegExp = /unable to generate smock spec from contract name*/; + await expect(smock.fake('Storage')).to.be.rejectedWith(Error, regex); + }); }); diff --git a/test/unit/mock/editable-storage-logic.spec.ts b/test/unit/mock/editable-storage-logic.spec.ts index a74576e..564111c 100644 --- a/test/unit/mock/editable-storage-logic.spec.ts +++ b/test/unit/mock/editable-storage-logic.spec.ts @@ -14,7 +14,7 @@ describe('Mock: Editable storage logic', () => { }); beforeEach(async () => { - mock = await storageGetterFactory.deploy(4); + mock = await storageGetterFactory.deploy(1); }); describe('setVariable', () => { @@ -62,8 +62,17 @@ describe('Mock: Editable storage logic', () => { expect(await mock.getAddress()).to.equal(ADDRESS_EXAMPLE); }); + it('should not be able to overwrite slot', async () => { + await mock.setVariable('_slotA', ADDRESS_EXAMPLE); + await mock.setVariable('_slotB', true); + + expect(await mock._slotA()).to.equal(ADDRESS_EXAMPLE); + expect(await mock._slotB()).to.equal(true); + }); + it('should be able to set an address in a packed storage slot', async () => { await mock.setVariable('_packedB', ADDRESS_EXAMPLE); + expect(await mock.getPackedAddress()).to.equal(ADDRESS_EXAMPLE); }); diff --git a/test/unit/mock/slot-overwrite.spec.ts b/test/unit/mock/slot-overwrite.spec.ts deleted file mode 100644 index d5b9c03..0000000 --- a/test/unit/mock/slot-overwrite.spec.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { MockContract, MockContractFactory, smock } from '@src'; -import { ADDRESS_EXAMPLE, ADDRESS_EXAMPLE2, ADDRESS_EXAMPLE3 } from '@test-utils'; -import { Bug, Bug__factory } from '@typechained'; -import { expect } from 'chai'; - -describe('Mock: Slot Overwrite', () => { - let bugFactory: MockContractFactory; - let mock: MockContract; - - before(async () => { - bugFactory = await smock.mock('Bug'); - }); - - beforeEach(async () => { - mock = await bugFactory.deploy(); - }); - - describe('setVariable', () => { - it('should not be able to overwrite slot', async () => { - await mock.setVariable('myAddress', ADDRESS_EXAMPLE); - await mock.setVariable('myBool', true); - await mock.setVariable('newAddress', ADDRESS_EXAMPLE2); - await mock.setVariable('newAddress2', ADDRESS_EXAMPLE3); - await mock.setVariable('newBool', false); - await mock.setVariable('newBool2', true); - expect(await mock.myAddress()).to.equal(ADDRESS_EXAMPLE); - expect(await mock.myBool()).to.equal(true); - expect(await mock.newAddress()).to.equal(ADDRESS_EXAMPLE2); - expect(await mock.newAddress2()).to.equal(ADDRESS_EXAMPLE3); - expect(await mock.newBool()).to.equal(false); - expect(await mock.newBool2()).to.equal(true); - }); - }); -}); diff --git a/test/utils/constants.ts b/test/utils/constants.ts index d1363bf..cf12ef5 100644 --- a/test/utils/constants.ts +++ b/test/utils/constants.ts @@ -2,10 +2,6 @@ import { utils } from 'ethers'; export const ADDRESS_EXAMPLE = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'; -export const ADDRESS_EXAMPLE2 = '0x4fd96172C40913c8100a47337f21DBf25C9e2fC6'; - -export const ADDRESS_EXAMPLE3 = '0x62eA762d9f2672f82d26b94f56849881831B9eaB'; - export const BYTES32_EXAMPLE = '0x1234123412341234123412341234123412341234123412341234123412341234'; export const BYTES_EXAMPLE = diff --git a/yarn.lock b/yarn.lock index efc8350..bdd33cc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -914,6 +914,13 @@ dependencies: "@types/node" "*" +"@types/chai-as-promised@7.1.5": + version "7.1.5" + resolved "https://registry.yarnpkg.com/@types/chai-as-promised/-/chai-as-promised-7.1.5.tgz#6e016811f6c7a64f2eed823191c3a6955094e255" + integrity sha512-jStwss93SITGBwt/niYrkf2C+/1KTeZCZl1LaeezTlqppAKeoQC7jxyqYuP72sxBGKCIbw7oHgbYssIRzT5FCQ== + dependencies: + "@types/chai" "*" + "@types/chai@*": version "4.2.21" resolved "https://registry.yarnpkg.com/@types/chai/-/chai-4.2.21.tgz#9f35a5643129df132cf3b5c1ec64046ea1af0650" From ddc39d784657280ebe6c1b94f53954e919d5bde9 Mon Sep 17 00:00:00 2001 From: mirazu Date: Wed, 6 Jul 2022 11:36:30 +0900 Subject: [PATCH 5/5] fix: slot-overwrite bug --- test/unit/mock/editable-storage-logic.spec.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/unit/mock/editable-storage-logic.spec.ts b/test/unit/mock/editable-storage-logic.spec.ts index 564111c..046b876 100644 --- a/test/unit/mock/editable-storage-logic.spec.ts +++ b/test/unit/mock/editable-storage-logic.spec.ts @@ -65,9 +65,36 @@ describe('Mock: Editable storage logic', () => { it('should not be able to overwrite slot', async () => { await mock.setVariable('_slotA', ADDRESS_EXAMPLE); await mock.setVariable('_slotB', true); + await mock.setVariable('_bytes', BYTES_EXAMPLE); + const value = utils.parseUnits('123'); + await mock.setVariable('_uint256', value); + await mock.setVariable('_bytes32', BYTES32_EXAMPLE); + const struct = { + packedA: true, + packedB: ADDRESS_EXAMPLE, + }; + await mock.setVariable('_packedStruct', struct); + const mapKey = 1234; + const mapValue = 5678; + await mock.setVariable('_uint256Map', { [mapKey]: mapValue }); + const mapKeyA = 1234; + const mapKeyB = 4321; + const mapVal = 5678; + + await mock.setVariable('_uint256NestedMap', { + [mapKeyA]: { + [mapKeyB]: mapVal, + }, + }); expect(await mock._slotA()).to.equal(ADDRESS_EXAMPLE); expect(await mock._slotB()).to.equal(true); + expect(await mock.getBytes()).to.equal(BYTES_EXAMPLE); + expect(await mock.getUint256()).to.equal(value); + expect(await mock.getBytes32()).to.equal(BYTES32_EXAMPLE); + expect(convertStructToPojo(await mock.getPackedStruct())).to.deep.equal(struct); + expect(await mock.getUint256MapValue(mapKey)).to.equal(mapValue); + expect(await mock.getNestedUint256MapValue(mapKeyA, mapKeyB)).to.equal(mapVal); }); it('should be able to set an address in a packed storage slot', async () => {