Fix error in unsafeAllowCustomTypes flag (#259)

Co-authored-by: Francisco Giordano <[email protected]>
OpenZeppelin · Dec 16, 2020 · 6870ad5 · 6870ad5
1 parent af339c3
commit 6870ad5
Show file tree

Hide file tree

Showing 8 changed files with 96 additions and 10 deletions.
diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259))
+
+Users of this flag are advised to update to this version.
+
 ## 1.4.1 (2020-11-30)
 
 - Fix a problem in deployment logic when used with Hyperledger Besu. ([#244](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/244))

diff --git a/packages/core/src/storage.ts b/packages/core/src/storage.ts
@@ -67,23 +67,25 @@ export function assertStorageUpgradeSafe(
   let errors = getStorageUpgradeErrors(original, updated);
 
   if (unsafeAllowCustomTypes) {
-    errors = errors
-      .filter(error => error.kind === 'typechange')
-      .filter(error => {
-        const { original, updated } = error;
-        if (original && updated) {
-          // Skip storage errors if the only difference seems to be the AST id number
-          return stabilizeTypeIdentifier(original?.type) !== stabilizeTypeIdentifier(updated?.type);
-        }
-        return error;
-      });
+    // Types with the same name are assumed compatible
+    errors = errors.filter(error => !isTypechange(error) || typechangePreservesNames(error));
   }
 
   if (errors.length > 0) {
     throw new StorageUpgradeErrors(errors);
   }
 }
 
+function isTypechange(error: StorageOperation): error is StorageOperation & { kind: 'typechange' } {
+  return error.kind === 'typechange';
+}
+
+function typechangePreservesNames(error: StorageOperation & { kind: 'typechange' }): boolean {
+  assert(error.updated !== undefined);
+  assert(error.original !== undefined);
+  return stabilizeTypeIdentifier(error.original.type) !== stabilizeTypeIdentifier(error.updated.type);
+}
+
 class StorageUpgradeErrors extends UpgradesError {
   constructor(readonly errors: StorageOperation[]) {
     super(`New storage layout is incompatible due to the following changes`, () => {

diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259))
+
+Users of this flag are advised to update to this version.
+
 ## 1.4.2 (2020-12-04)
 
 - Fix a bug that prevented some solc errors from reaching the console. ([#253](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/253))

diff --git a/packages/plugin-hardhat/contracts/PortfolioV2.sol b/packages/plugin-hardhat/contracts/PortfolioV2.sol
@@ -24,3 +24,27 @@ contract PortfolioV2 {
     }
 
 }
+
+contract PortfolioV2Bad {
+    struct Asset {
+        bool enabled;
+        uint amount;
+    }
+
+    uint insert;
+    mapping (string => Asset) assets;
+
+    function initialize() public view {
+        console.log("Deploying PortfolioV2");
+    }
+
+    function enable(string memory name) public returns (bool) {
+        if (assets[name].enabled) {
+            return false;
+        } else {
+            assets[name] = Asset(true, 10);
+            return true;
+        }
+    }
+
+}
diff --git a/packages/plugin-hardhat/test/happy-path-with-structs.js b/packages/plugin-hardhat/test/happy-path-with-structs.js
@@ -7,6 +7,7 @@ upgrades.silenceWarnings();
 test.before(async t => {
   t.context.Portfolio = await ethers.getContractFactory('Portfolio');
   t.context.PortfolioV2 = await ethers.getContractFactory('PortfolioV2');
+  t.context.PortfolioV2Bad = await ethers.getContractFactory('PortfolioV2Bad');
 });
 
 test('deployProxy without flag', async t => {
@@ -32,3 +33,12 @@ test('upgradeProxy with flag', async t => {
   const portfolio2 = await upgrades.upgradeProxy(portfolio.address, PortfolioV2, { unsafeAllowCustomTypes: true });
   await portfolio2.enable('ETH');
 });
+
+test('upgradeProxy with flag but incompatible layout', async t => {
+  const { Portfolio, PortfolioV2Bad } = t.context;
+  const portfolio = await upgrades.deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true });
+  const error = await t.throwsAsync(() =>
+    upgrades.upgradeProxy(portfolio.address, PortfolioV2Bad, { unsafeAllowCustomTypes: true }),
+  );
+  t.true(error.message.includes('Variable `assets` was replaced with `insert`'));
+});
diff --git a/packages/plugin-truffle/CHANGELOG.md b/packages/plugin-truffle/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259))
+
+Users of this flag are advised to update to this version.
+
 ## 1.3.0 (2020-11-24)
 
 - Add `silenceWarnings` to emit a single warning and silence all subsequent ones. ([#228](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/228))

diff --git a/packages/plugin-truffle/test/contracts/PortfolioV2.sol b/packages/plugin-truffle/test/contracts/PortfolioV2.sol
@@ -24,3 +24,27 @@ contract PortfolioV2 {
     }
 
 }
+
+contract PortfolioV2Bad {
+    struct Asset {
+        bool enabled;
+        uint amount;
+    }
+
+    uint insert;
+    mapping (string => Asset) assets;
+
+    function initialize() public view {
+        console.log("Deploying PortfolioV2");
+    }
+
+    function enable(string memory name) public returns (bool) {
+        if (assets[name].enabled) {
+            return false;
+        } else {
+            assets[name] = Asset(true, 10);
+            return true;
+        }
+    }
+
+}
diff --git a/packages/plugin-truffle/test/test/happy-path-with-structs.js b/packages/plugin-truffle/test/test/happy-path-with-structs.js
@@ -4,6 +4,7 @@ const { deployProxy, upgradeProxy } = require('@openzeppelin/truffle-upgrades');
 
 const Portfolio = artifacts.require('Portfolio');
 const PortfolioV2 = artifacts.require('PortfolioV2');
+const PortfolioV2Bad = artifacts.require('PortfolioV2Bad');
 
 contract('PortfolioWithoutFlag', function () {
   it('deployProxy', async function () {
@@ -20,4 +21,11 @@ contract('PortfolioWithFlag', function () {
     const portfolio = await deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true });
     await upgradeProxy(portfolio.address, PortfolioV2, { unsafeAllowCustomTypes: true });
   });
+
+  it('upgradeProxy with flag but incompatible layout', async function () {
+    const portfolio = await deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true });
+    await assert.rejects(upgradeProxy(portfolio.address, PortfolioV2Bad, { unsafeAllowCustomTypes: true }), error =>
+      error.message.includes('Variable `assets` was replaced with `insert`'),
+    );
+  });
 });