From 7274d735b88c29f8bbb07caa5bf873ddc37273ad Mon Sep 17 00:00:00 2001 From: Erik Weitenberg Date: Wed, 29 Apr 2020 10:07:48 +0200 Subject: [PATCH 1/4] Port the cache bug test --- test/cacheBugTest.js | 64 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 test/cacheBugTest.js diff --git a/test/cacheBugTest.js b/test/cacheBugTest.js new file mode 100644 index 0000000..7f475a2 --- /dev/null +++ b/test/cacheBugTest.js @@ -0,0 +1,64 @@ +const etherlime = require('etherlime-lib'); + +const Test1Facet = require('../build/Test1Facet.json'); +const DiamondExample = require('../build/DiamondExample.json'); +const DiamondLoupeFacet = require('../build/DiamondLoupeFacet.json'); +const DiamondFacet = require('../build/DiamondFacet.json'); + + +describe('Cache bug', () => { + let aliceAccount = accounts[0]; + let deployer; + let test1Facet; + let test2Facet; + let diamondFacet; + let diamondLoupeFacet; + let diamondExample; + + const zeroAddress = "0x0000000000000000000000000000000000000000"; + + // Selectors without 0x + // web3.eth.abi.encodeFunctionSignature("test1Func2()").slice(2) etc + const sel0 = '19e3b533'; + const sel1 = '0716c2ae'; + const sel2 = '11046047'; + const sel3 = 'cf3bbe18'; + + before(async () => { + deployer = new etherlime.EtherlimeGanacheDeployer(aliceAccount.secretKey); + diamondExample = await deployer.deploy(DiamondExample); + test1Facet = await deployer.deploy(Test1Facet); + diamondLoupeFacet = deployer.wrapDeployedContract(DiamondLoupeFacet, diamondExample.contractAddress); + diamondFacet = deployer.wrapDeployedContract(DiamondFacet, diamondExample.contractAddress); + + // Add facet + let newFacetDescription = test1Facet.contractAddress + sel0 + sel1 + sel2 + sel3; + await diamondFacet.diamondCut([newFacetDescription]); + + // Remove facet + let removalDescription = zeroAddress + sel1; + await diamondFacet.diamondCut([removalDescription]); + }); + + // If the bug is present, this should leave the last slot in cache as + // [ sel0 sel3 sel2 ] (sel3 off the edge) + // but not write it back, so the storage should still have + // [ sel0 sel1 sel2 ] (sel3 off the edge) + + it("should not exhibit the cache bug", async () => { + // Get the test1Facet's registered functions + let selectors = await diamondLoupeFacet.facetFunctionSelectors(test1Facet.contractAddress); + console.log(selectors); + console.log([sel0, sel1, sel2, sel3].join(" ")); + + // Short-circuit test for our specific bug + assert.isFalse((!selectors.includes(sel3)) && selectors.includes(sel1), "Exhibits the cache bug"); + + // Check individual correctness + assert.isTrue(selectors.includes(sel0), "Does not contain sel0"); + assert.isTrue(selectors.includes(sel2), "Does not contain sel2"); + assert.isTrue(selectors.includes(sel3), "Does not contain sel3"); + + assert.isFalse(selectors.includes(sel1), "Contains sel1"); + }); +}); \ No newline at end of file From 446a2b30a9ef24619dae4742f2b499c8e0fc3fe1 Mon Sep 17 00:00:00 2001 From: Erik Weitenberg Date: Wed, 29 Apr 2020 10:29:32 +0200 Subject: [PATCH 2/4] Adjust test for fewer slots in the example --- test/cacheBugTest.js | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/test/cacheBugTest.js b/test/cacheBugTest.js index 7f475a2..1945064 100644 --- a/test/cacheBugTest.js +++ b/test/cacheBugTest.js @@ -5,7 +5,11 @@ const DiamondExample = require('../build/DiamondExample.json'); const DiamondLoupeFacet = require('../build/DiamondLoupeFacet.json'); const DiamondFacet = require('../build/DiamondFacet.json'); - +// The standard diamond example comes with 6 full slots. +// [cut, loupe, loupe, loupe, loupe, erc165] +// This bug manifests if you delete something from the final +// selector slot array, so we'll fill up this slot with 2 more +// things, and have a fresh row to work with. describe('Cache bug', () => { let aliceAccount = accounts[0]; let deployer; @@ -19,10 +23,12 @@ describe('Cache bug', () => { // Selectors without 0x // web3.eth.abi.encodeFunctionSignature("test1Func2()").slice(2) etc - const sel0 = '19e3b533'; - const sel1 = '0716c2ae'; - const sel2 = '11046047'; + const sel0 = '19e3b533'; // fills up slot 0 + const sel1 = '0716c2ae'; // fills up slot 0 + const sel2 = '11046047'; // all others into slot 1 const sel3 = 'cf3bbe18'; + const sel4 = '24c1d5a7'; + const sel5 = 'cbb835f6'; before(async () => { deployer = new etherlime.EtherlimeGanacheDeployer(aliceAccount.secretKey); @@ -32,33 +38,32 @@ describe('Cache bug', () => { diamondFacet = deployer.wrapDeployedContract(DiamondFacet, diamondExample.contractAddress); // Add facet - let newFacetDescription = test1Facet.contractAddress + sel0 + sel1 + sel2 + sel3; + let newFacetDescription = test1Facet.contractAddress + sel0 + sel1 + sel2 + sel3 + sel4 + sel5; await diamondFacet.diamondCut([newFacetDescription]); // Remove facet - let removalDescription = zeroAddress + sel1; + let removalDescription = zeroAddress + sel3; await diamondFacet.diamondCut([removalDescription]); }); // If the bug is present, this should leave the last slot in cache as - // [ sel0 sel3 sel2 ] (sel3 off the edge) + // [ sel2 sel5 sel4 ] (sel5 off the edge) // but not write it back, so the storage should still have - // [ sel0 sel1 sel2 ] (sel3 off the edge) + // [ sel2 sel3 sel4 ] (sel5 off the edge) it("should not exhibit the cache bug", async () => { // Get the test1Facet's registered functions let selectors = await diamondLoupeFacet.facetFunctionSelectors(test1Facet.contractAddress); console.log(selectors); - console.log([sel0, sel1, sel2, sel3].join(" ")); - - // Short-circuit test for our specific bug - assert.isFalse((!selectors.includes(sel3)) && selectors.includes(sel1), "Exhibits the cache bug"); + console.log([sel0, sel1, sel2, sel3, sel4, sel5].join(" ")); // Check individual correctness assert.isTrue(selectors.includes(sel0), "Does not contain sel0"); + assert.isTrue(selectors.includes(sel1), "Does not contain sel1"); assert.isTrue(selectors.includes(sel2), "Does not contain sel2"); - assert.isTrue(selectors.includes(sel3), "Does not contain sel3"); + assert.isTrue(selectors.includes(sel4), "Does not contain sel4"); + assert.isTrue(selectors.includes(sel5), "Does not contain sel5"); - assert.isFalse(selectors.includes(sel1), "Contains sel1"); + assert.isFalse(selectors.includes(sel3), "Contains sel3"); }); }); \ No newline at end of file From a79bcbbc6619a55262e2a8e4fcbc14fe3db16f05 Mon Sep 17 00:00:00 2001 From: Erik Weitenberg Date: Wed, 29 Apr 2020 10:31:38 +0200 Subject: [PATCH 3/4] Remove unnecessary logging --- test/cacheBugTest.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cacheBugTest.js b/test/cacheBugTest.js index 1945064..17c8081 100644 --- a/test/cacheBugTest.js +++ b/test/cacheBugTest.js @@ -54,8 +54,8 @@ describe('Cache bug', () => { it("should not exhibit the cache bug", async () => { // Get the test1Facet's registered functions let selectors = await diamondLoupeFacet.facetFunctionSelectors(test1Facet.contractAddress); - console.log(selectors); - console.log([sel0, sel1, sel2, sel3, sel4, sel5].join(" ")); + // console.log(selectors); + // console.log([sel0, sel1, sel2, sel3, sel4, sel5].join(" ")); // Check individual correctness assert.isTrue(selectors.includes(sel0), "Does not contain sel0"); From a07f99d94088b2a85127588d34f56650a8e41497 Mon Sep 17 00:00:00 2001 From: Erik Weitenberg Date: Wed, 29 Apr 2020 10:35:03 +0200 Subject: [PATCH 4/4] Fix the cache bug On removal of a selector from the last slot, the entire operation happens in the cache (named slot.selectorSlot). There is a flag to mark it 'dirty', called slot.newSlot, which is used correctly when adding new selectors. However it is not changed upon removal, and if you only remove a slot, the cache is not written back. This causes the storage to still contain the removed selector. Final slot before removal: [ sel2 sel3 sel4 sel5 ] Final slot after removal of sel3, cache: [ sel2 sel5 sel4 ] Final slot in storage after removal: [ sel2 sel3 sel4 ] By setting the dirty flag to true, the cache is written back correctly. --- contracts/DiamondFacet.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/DiamondFacet.sol b/contracts/DiamondFacet.sol index 34d91fa..c2ef483 100644 --- a/contracts/DiamondFacet.sol +++ b/contracts/DiamondFacet.sol @@ -106,6 +106,7 @@ contract DiamondFacet is Diamond, DiamondStorageContract { else { slot.selectorSlot = slot.selectorSlot & ~(CLEAR_SELECTOR_MASK >> slot.oldSelectorSlotIndex * 32) | bytes32(lastSelector) >> slot.oldSelectorSlotIndex * 32; selectorSlotLength--; + slot.newSlot = true; } if(selectorSlotLength == 0) { delete ds.selectorSlots[selectorSlotsLength];