From 54b00d43bfe83b2280faf0557cf23285d5676a6b Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 5 Jun 2024 17:58:51 -0400 Subject: [PATCH 01/22] Fix function pointer comparison in storage layouts --- packages/core/contracts/test/Storage.sol | 18 ++++++++++++++++++ packages/core/src/storage.test.ts | 10 +++++++++- packages/core/src/storage/compare.ts | 3 +-- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/packages/core/contracts/test/Storage.sol b/packages/core/contracts/test/Storage.sol index 2cab6e46a..e44077b34 100644 --- a/packages/core/contracts/test/Storage.sol +++ b/packages/core/contracts/test/Storage.sol @@ -899,4 +899,22 @@ contract V2Storage { } contract StorageUpgrade_ConsumeAndAddGap_Storage_V2 is V2Storage, Parent1, Parent2 { +} + +contract StorageUpgrade_FunctionPointer_V1 { + struct S { + uint256 a; + function(bool) external b; + } + + S private s; +} + +contract StorageUpgrade_FunctionPointer_V2_Ok { + struct S { + uint256 a; + function(bool) external b; + } + + S private s; } \ No newline at end of file diff --git a/packages/core/src/storage.test.ts b/packages/core/src/storage.test.ts index 5ca4499ac..6e67b2c89 100644 --- a/packages/core/src/storage.test.ts +++ b/packages/core/src/storage.test.ts @@ -4,11 +4,12 @@ import { findAll, astDereferencer } from 'solidity-ast/utils'; import { artifacts } from 'hardhat'; import { SolcOutput } from './solc-api'; -import { getStorageUpgradeErrors } from './storage'; +import { assertStorageUpgradeSafe, getStorageUpgradeErrors } from './storage'; import { StorageLayout } from './storage/layout'; import { extractStorageLayout } from './storage/extract'; import { stabilizeTypeIdentifier } from './utils/type-id'; import { stabilizeStorageLayout } from './utils/stabilize-layout'; +import { withValidationDefaults } from './validate'; interface Context { extractStorageLayout: (contract: string) => ReturnType; @@ -937,3 +938,10 @@ test('storage upgrade with struct gap', t => { }, }); }); + +test('storage upgrade with function pointers', t => { + const v1 = t.context.extractStorageLayout('StorageUpgrade_FunctionPointer_V1'); + const v2_Ok = t.context.extractStorageLayout('StorageUpgrade_FunctionPointer_V2_Ok'); + + t.deepEqual(getStorageUpgradeErrors(v1, v2_Ok), []); +}); \ No newline at end of file diff --git a/packages/core/src/storage/compare.ts b/packages/core/src/storage/compare.ts index 2e3f67b4a..f7a96669f 100644 --- a/packages/core/src/storage/compare.ts +++ b/packages/core/src/storage/compare.ts @@ -191,8 +191,7 @@ export class StorageLayoutComparator { const re = /^t_function_(internal|external)/; const originalVisibility = original.head.match(re); const updatedVisibility = updated.head.match(re); - assert(originalVisibility && updatedVisibility); - if (originalVisibility[0] !== updatedVisibility[0]) { + if (originalVisibility && updatedVisibility && originalVisibility[0] !== updatedVisibility[0]) { return { kind: 'visibility change', original, updated }; } } From 53687a70299b70f21d263124268c3ab3202b46ae Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 5 Jun 2024 18:18:50 -0400 Subject: [PATCH 02/22] Add negative test --- packages/core/contracts/test/Storage.sol | 9 ++++++++ packages/core/src/storage.test.ts | 27 +++++++++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/packages/core/contracts/test/Storage.sol b/packages/core/contracts/test/Storage.sol index e44077b34..d7b53a9dc 100644 --- a/packages/core/contracts/test/Storage.sol +++ b/packages/core/contracts/test/Storage.sol @@ -916,5 +916,14 @@ contract StorageUpgrade_FunctionPointer_V2_Ok { function(bool) external b; } + S private s; +} + +contract StorageUpgrade_FunctionPointer_V2_Bad { + struct S { + uint256 a; + function(bool) internal b; + } + S private s; } \ No newline at end of file diff --git a/packages/core/src/storage.test.ts b/packages/core/src/storage.test.ts index 6e67b2c89..08c7aa411 100644 --- a/packages/core/src/storage.test.ts +++ b/packages/core/src/storage.test.ts @@ -4,12 +4,11 @@ import { findAll, astDereferencer } from 'solidity-ast/utils'; import { artifacts } from 'hardhat'; import { SolcOutput } from './solc-api'; -import { assertStorageUpgradeSafe, getStorageUpgradeErrors } from './storage'; +import { getStorageUpgradeErrors } from './storage'; import { StorageLayout } from './storage/layout'; import { extractStorageLayout } from './storage/extract'; import { stabilizeTypeIdentifier } from './utils/type-id'; import { stabilizeStorageLayout } from './utils/stabilize-layout'; -import { withValidationDefaults } from './validate'; interface Context { extractStorageLayout: (contract: string) => ReturnType; @@ -942,6 +941,28 @@ test('storage upgrade with struct gap', t => { test('storage upgrade with function pointers', t => { const v1 = t.context.extractStorageLayout('StorageUpgrade_FunctionPointer_V1'); const v2_Ok = t.context.extractStorageLayout('StorageUpgrade_FunctionPointer_V2_Ok'); + const v2_Bad = t.context.extractStorageLayout('StorageUpgrade_FunctionPointer_V2_Bad'); t.deepEqual(getStorageUpgradeErrors(v1, v2_Ok), []); -}); \ No newline at end of file + + t.like(getStorageUpgradeErrors(v1, v2_Bad), { + length: 1, + 0: { + kind: 'typechange', + change: { + kind: 'struct members', + ops: { + length: 1, + 0: { + kind: 'typechange', + change: { + kind: 'visibility change', + }, + }, + }, + }, + original: { label: 's' }, + updated: { label: 's' }, + }, + }); +}); From 461a1b5e800cb585f008109c23e8e9e4ced9bac0 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 5 Jun 2024 18:21:02 -0400 Subject: [PATCH 03/22] Update changelog --- packages/core/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 374713036..9a07866cf 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix storage layout comparison for function pointers in structs. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) + ## 1.33.1 (2024-04-25) - Fix Hardhat compile error when variable has whitespace before semicolon. ([#1020](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1020)) From 0f9148e892ebcb7062e0c2056b1d1269dc06a5cf Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Thu, 6 Jun 2024 16:50:25 -0400 Subject: [PATCH 04/22] Only get visibility change if both are functions --- packages/core/contracts/test/Storage.sol | 2 +- packages/core/src/storage/compare.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core/contracts/test/Storage.sol b/packages/core/contracts/test/Storage.sol index d7b53a9dc..b40d85f5e 100644 --- a/packages/core/contracts/test/Storage.sol +++ b/packages/core/contracts/test/Storage.sol @@ -922,7 +922,7 @@ contract StorageUpgrade_FunctionPointer_V2_Ok { contract StorageUpgrade_FunctionPointer_V2_Bad { struct S { uint256 a; - function(bool) internal b; + function(bool) b; } S private s; diff --git a/packages/core/src/storage/compare.ts b/packages/core/src/storage/compare.ts index f7a96669f..5ce260458 100644 --- a/packages/core/src/storage/compare.ts +++ b/packages/core/src/storage/compare.ts @@ -191,7 +191,8 @@ export class StorageLayoutComparator { const re = /^t_function_(internal|external)/; const originalVisibility = original.head.match(re); const updatedVisibility = updated.head.match(re); - if (originalVisibility && updatedVisibility && originalVisibility[0] !== updatedVisibility[0]) { + assert(originalVisibility && updatedVisibility); + if (originalVisibility[0] !== updatedVisibility[0]) { return { kind: 'visibility change', original, updated }; } } @@ -282,7 +283,7 @@ export class StorageLayoutComparator { updated: ParsedTypeDetailed, { allowAppend }: { allowAppend: boolean }, ): TypeChange | undefined { - if (updated.head.startsWith('t_function')) { + if (original.head.startsWith('t_function') && updated.head.startsWith('t_function')) { return this.getVisibilityChange(original, updated); } From 93bf49dd6cdb75a560c1d73d0a333c51cbedd2a9 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Thu, 6 Jun 2024 17:53:17 -0400 Subject: [PATCH 05/22] Add check for internal function in struct --- packages/core/contracts/test/Validations.sol | 18 ++++++++++++++++++ packages/core/src/validate.test.ts | 5 +++++ packages/core/src/validate/overrides.ts | 5 +++++ packages/core/src/validate/report.ts | 7 +++++++ packages/core/src/validate/run.ts | 20 +++++++++++++++++++- 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/packages/core/contracts/test/Validations.sol b/packages/core/contracts/test/Validations.sol index cbec23c36..507995bd4 100644 --- a/packages/core/contracts/test/Validations.sol +++ b/packages/core/contracts/test/Validations.sol @@ -206,3 +206,21 @@ contract TransitiveLibraryIsUnsafe { DirectLibrary.f2(); } } + +contract ExternalFunctionPointer { + struct S { + function(bool) external foo; + } +} + +contract InternalFunctionPointer { + struct S { + function(bool) internal foo; + } +} + +contract ImpliedInternalFunctionPointer { + struct S { + function(bool) foo; + } +} \ No newline at end of file diff --git a/packages/core/src/validate.test.ts b/packages/core/src/validate.test.ts index fb9f2793a..50e75de68 100644 --- a/packages/core/src/validate.test.ts +++ b/packages/core/src/validate.test.ts @@ -140,6 +140,11 @@ testValid('TransitiveLibraryIsUnsafe', 'transparent', false); testValid('contracts/test/ValidationsSameNameSafe.sol:SameName', 'transparent', true); testValid('contracts/test/ValidationsSameNameUnsafe.sol:SameName', 'transparent', false); +testValid('ExternalFunctionPointer', 'transparent', true); +testValid('InternalFunctionPointer', 'transparent', false); +testValid('ImpliedInternalFunctionPointer', 'transparent', false); +testOverride('ImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['struct-internal-function'] }, true); + test('ambiguous name', t => { const error = t.throws(() => getContractVersion(t.context.validation, 'SameName')); t.is( diff --git a/packages/core/src/validate/overrides.ts b/packages/core/src/validate/overrides.ts index 6e9648114..7affb2028 100644 --- a/packages/core/src/validate/overrides.ts +++ b/packages/core/src/validate/overrides.ts @@ -76,6 +76,11 @@ export const ValidationErrorUnsafeMessages: Record { diff --git a/packages/core/src/validate/report.ts b/packages/core/src/validate/report.ts index 2b72726ce..f92c61552 100644 --- a/packages/core/src/validate/report.ts +++ b/packages/core/src/validate/report.ts @@ -64,6 +64,13 @@ const errorInfo: ErrorDescriptions = { ` @openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol`, link: 'https://zpl.in/upgrades/error-008', }, + 'struct-internal-function': { + msg: e => `Struct has internal function \`${e.name}\``, + hint: () => + `Use external functions to avoid code pointers that will no longer be valid after an upgrade\n` + + ` or skip this check with the \`unsafeAllow.struct-internal-function\` flag and\n` + + ` ensure you always reassign the internal functions in structs during upgrades`, + }, }; function describeError(e: ValidationError, color = true): string { diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index ea97b1816..c711fb5b0 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -40,6 +40,7 @@ export const errorKinds = [ 'delegatecall', 'selfdestruct', 'missing-public-upgradeto', + 'struct-internal-function', ] as const; export type ValidationError = @@ -60,7 +61,8 @@ interface ValidationErrorWithName extends ValidationErrorBase { | 'state-variable-immutable' | 'external-library-linking' | 'struct-definition' - | 'enum-definition'; + | 'enum-definition' + | 'struct-internal-function'; } interface ValidationErrorConstructor extends ValidationErrorBase { @@ -197,6 +199,7 @@ export function validate( // TODO: add linked libraries support // https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/52 ...getLinkingErrors(contractDef, bytecode), + ...getStructFunctionErrors(contractDef, decodeSrc), ]; validation[key].layout = extractStorageLayout( @@ -557,3 +560,18 @@ function* getLinkingErrors( } } } + +function* getStructFunctionErrors(contractDef: ContractDefinition, decodeSrc: SrcDecoder): Generator { + // Note: Solidity currently does not include struct annotations in the AST, so this check cannot be skipped with annotations + for (const structDef of findAll('StructDefinition', contractDef)) { + for (const member of structDef.members) { + if (member.typeName?.nodeType === 'FunctionTypeName' && member.typeName.visibility === 'internal') { + yield { + kind: 'struct-internal-function', + name: member.name, + src: decodeSrc(member), + }; + } + } + } +} From 5e3ddff863738c62fb3c602b8feda02bb289e307 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Thu, 6 Jun 2024 17:59:15 -0400 Subject: [PATCH 06/22] Update docs and snapshots --- docs/modules/ROOT/pages/api-core.adoc | 2 +- .../ROOT/pages/api-hardhat-upgrades.adoc | 1 + docs/modules/ROOT/pages/faq.adoc | 1 + packages/core/src/cli/cli.test.ts.md | 4 ++-- packages/core/src/cli/cli.test.ts.snap | Bin 2137 -> 2142 bytes 5 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/modules/ROOT/pages/api-core.adoc b/docs/modules/ROOT/pages/api-core.adoc index d6c2ea887..11baf41ed 100644 --- a/docs/modules/ROOT/pages/api-core.adoc +++ b/docs/modules/ROOT/pages/api-core.adoc @@ -119,7 +119,7 @@ If any errors are found, the command will exit with a non-zero exit code and pri * `--contract ` The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. * `--reference ` Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. * `--requireReference` Can only be used when the `--contract` option is also provided. Not compatible with `--unsafeSkipStorageCheck`. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. -* `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto` +* `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function` * `--unsafeAllowRenames` - Configure storage layout check to allow variable renaming. * `--unsafeSkipStorageCheck` - Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index c6f215e61..9cfb69eb4 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -16,6 +16,7 @@ The following options are common to some functions. ** `"constructor"`: Allows defining a constructor. See `constructorArgs`. ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?] ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` or `upgradeToAndCall` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism. +** `"struct-internal-function"`: Allow internal functions in structs. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming. * `unsafeSkipStorageCheck`: (`boolean`) upgrades the proxy or beacon without first checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. * `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables. diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index 74e36a68a..c1ea3d494 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -50,6 +50,7 @@ Deployment and upgrade related functions come with an optional `opts` object, wh * `delegatecall` * `selfdestruct` * `missing-public-upgradeto` + * `struct-internal-function` This function is a generalized version of the original `unsafeAllowCustomTypes` and `unsafeAllowLinkedLibraries` allowing any check to be manually disabled. diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md index b6ab145f9..265117f3b 100644 --- a/packages/core/src/cli/cli.test.ts.md +++ b/packages/core/src/cli/cli.test.ts.md @@ -19,7 +19,7 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto␊ + --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` @@ -39,7 +39,7 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto␊ + --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index 693babc3014f0d082aa7228c0526e232596178c4..07907c06b30441de92d5f57eed76ffe88aa60cb5 100644 GIT binary patch delta 2140 zcmV-i2&4De5Z(}fK~_N^Q*L2!b7*gLAa*kf0{}1{F=3AwAsLZhS=#R0sf$9(JA@e= zJ(#X*tUR+{OtT-02mk;800003?OM%l8%GkB5(Eag$YGE1WgA&+Yy%w8l;zka6u~q_ z!SpI*LW)L=z>u1p9=8envcxFv=^TH}bJfq%{;iK_{3|5J1q^+mHIK z{oTV>d#`hUxZCRPJcmo2FAiFrc5ml-skBQ1G7Kb&x8ngLXefB#V|CT>g^Ms6gp}Ec zaz`MW`mkR36NI^)s*&jBm;__uBlrt$fd#QZB(Xy_N~O9#3SGhjSt}tlYZF2pFQ9^B zhCCdMQ6Q9@#cE%U>2Rz~#FR_)q}OR9hcaaXg{?h*0T$6<0xMh(F8i?G3j=Mf2m|=K zfp&?5Lk8}Fo*UQ?sH2>%XmyrScLO=XM~|^@k8yxJUwor6f@?QG#@xa^5po+$OBQaR zmV+iD^ph(nlwco5!%zkQXk^FKr?Og%T)2s(M~2jyAPVXhx(^sDI3v6?n3k2q3vD+# z?SpQAy>S4D9sqzGyF^*-gp5tlix8saP-4f|9Sw528Nf)wsU6@VF$cDk5oT-&Nhm)j zS`@YorhK$Fw8cJgzz;qoXuI3oYj&IMM)S}xzJWOs9BhWIfSs-A6=^ZN6&eFX0a+%6 z!uo{Tpdq*GQ!7jmK%b5(gvy6pLkvT70?j9X>{AJ-FM>x$SP6X?1-T~9tTZK~G~%!Y zM|H=rV09W)Q6_KvC7 zi-2g1$?#aaZnz!WA`ziU`e(u$D+O4=v_z$f%PjTA_=G@)~=fd-prP3a(k<8fT=kkNCduA+8PKy4aEgBCYU7 zj6zOoK+KIXeH@{*Lx5_)6U}E z2ozkAkqkxmWyuHhq)Z*{Nwf~eKxV~fWMe)ez6fPPIG11^#I(~Hfz>jGA)LT}R^bO5 zrMU2pI9TnEw*MIJ$aj_V>gy%8%>pIvNiiBi|fCWH(o6c^#$6ZRowo+;uaqWau2F_zyC!K-ArUf(?yYE>Q>%=aNpakp_U6Q zQ8gGReNS8o;S7puAvy{Z$v3fWLt&7&j#QPOUaxbz0pKLJ%Yz9hghfWFOg=F|k#e$c zG|EtfC?B%WPP>nRPz{A{B2=##i_(p>o*EMG4B^>XFLmECI`Rs%SErLo7*HlFGe#&- z3Iz9l-nwH3_rS+oLgNH~h+iPaM$x;Kn_5we!tJ=7*w@WMQy1}~Cd4A1R&#Ug!M#5q zrSfTpWZq92FV*?jx@S5%JRD)~2t0?5Zu9P^UvE8lj;tMI{JfQ#W1$`s#(srSRdIDQ zU9tlmbbz!_nCN{xQMV;T2yNvi$Rh%+8H&Q+~kE_)Oe@Jb} z!H%S+*9TIsGtAE&Za7Qp>1-%3hFtOFuWsGGs_d`cx%UpF|6aYd-`qtvt6NK+3fcO> zqIyQ~De_<-R^RS_BL`DPY=9XiDDb?7;wkL(x-o{SB5lZ;o5#NTH8YO=vSy59IVh9O zll&2X^8t@QoCm{N4n|o|QJH5W^YiNZe=-35d%YMytGasSi8arO&(7n7@xbi_c2O0c z*Ky=}h9iHvN{*Pd_A2PH4D-Ewn1kvt^kg|d`0ss?0sr@Zg5Y1a$GOlfCg*xwLuy5yry}a-p(t@YNt7u#* z8Vw^FbFa88INZ&Um`#v+_@E*PRb;+Z{N9Yo`q|r`T6_IQquJ{vuNzLAmvl3Fi8Al# zZyECaTCgW!erKoVWo+3xA4Ub7Z~R_@+Ozde2DXO&d zK0cep;2Ei70^(0ke6`B z5JX^x{U3`600000000B+TFq`7M-rA21O^!7u*c+*MuLrPfFp{s9NUCeF-=i0y;?F$ zipCm&AvHNYWUqR<$I~;isM7{}*<6Dha@b4uHTDG-d4b$>$Rp&O>Yg9=@JF7J9Lo!k zfNgT7yMDgh9~md|B!$NjM!tsJSsW%mO_z2l&~IFWz0dv8F!j z@2&meoy{A4iASV{xOavg3Xkw};(3HoUJ1RCk8L8Yq3{Vh#f*Xgg1*{$)NdW^9ktr~ zouj>0clSA5>U??F>a=^i&r78}5|CjaQM?@w7(qk910SoajxSt<(IBMEMwB}O+0=*i z!k-|_?Np6KFUKSp6Cc4}a0@Jm1tN(ZvQaA4{ZZ%=9>`h=p;?;{>UaSa95dwMV2lEx z4YWrb95QeZ z^xVXLKpo|5MXR%vx|_%mK6;FOdyE6*`Qq;yBe-@GWXvtx7a_O7v}EB1YB^{kLO;2J zLJ9U^Gz?_`fJSyqeJZQP$c39odSpnQ38J8Gq5FWbf-}NPgK1ewywFag(?0Ch8;5}C zAppp+OO(}4$k+tE2q9VyC3bw>(IBUr0gNP^+5s*Sb6`suVaAq_gz^ibMPb`u%13)c zTkI1D{NO`^cDl{|X1CdHG>;798<-=(!DiSB*x8C+kru;Sp)o)dkY!RRtWT*88gjcn zwZa4e^y#QVsC>vZ#4toB(0sx^m4Nypcyxr7(1%fwYvSBWQ!+{;4qI?!vI7BDhTFuU zoY)D3Q!^Cr;QoM6KAH#bMF;?QQ|qq?S6js11yluAr;%laTxD+WgnGRQh{l)4hzU?Dvv0E|tgX070w6|QkcD*K4<3m@Xz5Uz_YNg&b+Z^S6%qz1&?7}LiQ zN;?Fo23&#V)+z8qX|b@Bbi~zhSDw8^T{jHWw=yDU0rBC?vlt!t*q^|^`~<|QJ$^V0 zEX@w841~ozbTbzKyl9#5%fKT>MxbX1Sqj?;a{xGfw~Sm0*6@+#g#$*139Nybg|SOi zZb)F0;0_&y;OJ}$C zRjfre<|E>ZP$sl;3FbjeF3o$amN5)L15OG**eJ#2b;QAHeryG-+Q(S7R3PEnIq?hZ>POK#6nfA1#r8C&lGKYf8PTcx@LdK3+s3+a5O}gyshoRk!{}`)I&{|NW;L(-e}OtrJw}T8 zdl&reOP%OuW+@h`V_(ys6tQQnXx9*t1J@he`&>jKemx!@Z z^ls;-R@9M>#LR~S_lSGUq7yU-^GNDGCz-NzGkTSA1;c5Z?^ zBG9@KsXCyd5y>9(xk(Xge1z>7qJnA-i&H8X*J{wIFrp@aE1u>eaR`& z^IXVP1Deem?^6bafj}O(8fX|T!W3T#lA32yZVgt-DL4oHr z6i;Di9~onqD$<6mxq0kwKV`&$o#yz z{$mE9|E?DUXjNCQJ+bCF@u!P8VLVnlg<1ALgJs z3_V%S5B@vvWx)TnAoy49aV|AeRjbw2P<6GTY8lV|n2To%a`6J^j z$^r$&FE4ykwBRZ6DjHXcM#G54+>0y=4tFyoW)q|yKBx#n6`5}p z|7FHx{rt^Ot-XGu(d_k-*9~XQE4mrIM45N=mkjxSD%cahwNvvlwrpPvqXN!1elJ1o z*?K1f+rxr6o210n5g>Qe7Sl=eO^l##&m1@(pRHo?Sw#6$(=Jeg9YbX&X^Me185$rF8X9|J#Yx* Date: Thu, 6 Jun 2024 18:00:55 -0400 Subject: [PATCH 07/22] Update changelog --- packages/core/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 9a07866cf..ab4effb80 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Fix storage layout comparison for function pointers in structs. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) +- Fix storage layout comparison for function types in structs, disallow internal functions in structs. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) ## 1.33.1 (2024-04-25) From 633ebfb53c02a811156e3f9abb41b1a072a14ab0 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 7 Jun 2024 18:11:26 -0400 Subject: [PATCH 08/22] Check for internal functions in storage variables --- docs/modules/ROOT/pages/api-core.adoc | 2 +- .../ROOT/pages/api-hardhat-upgrades.adoc | 2 +- docs/modules/ROOT/pages/faq.adoc | 2 +- packages/core/contracts/test/Storage.sol | 6 ++--- packages/core/contracts/test/Validations.sol | 20 ++++++++++++--- packages/core/src/cli/cli.test.ts.md | 4 +-- packages/core/src/cli/cli.test.ts.snap | Bin 2142 -> 2148 bytes packages/core/src/storage.test.ts | 10 +++++++- packages/core/src/validate.test.ts | 7 +++++- packages/core/src/validate/overrides.ts | 6 ++--- packages/core/src/validate/report.ts | 6 ++--- packages/core/src/validate/run.ts | 23 +++++++++++++----- 12 files changed, 62 insertions(+), 26 deletions(-) diff --git a/docs/modules/ROOT/pages/api-core.adoc b/docs/modules/ROOT/pages/api-core.adoc index 11baf41ed..ae6413edb 100644 --- a/docs/modules/ROOT/pages/api-core.adoc +++ b/docs/modules/ROOT/pages/api-core.adoc @@ -119,7 +119,7 @@ If any errors are found, the command will exit with a non-zero exit code and pri * `--contract ` The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. * `--reference ` Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. * `--requireReference` Can only be used when the `--contract` option is also provided. Not compatible with `--unsafeSkipStorageCheck`. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. -* `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function` +* `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage` * `--unsafeAllowRenames` - Configure storage layout check to allow variable renaming. * `--unsafeSkipStorageCheck` - Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 9cfb69eb4..09d67693a 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -16,7 +16,7 @@ The following options are common to some functions. ** `"constructor"`: Allows defining a constructor. See `constructorArgs`. ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?] ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` or `upgradeToAndCall` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism. -** `"struct-internal-function"`: Allow internal functions in structs. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. +** `"internal-function-storage"`: Allow internal functions in storage variables. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming. * `unsafeSkipStorageCheck`: (`boolean`) upgrades the proxy or beacon without first checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. * `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables. diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index c1ea3d494..890c82474 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -50,7 +50,7 @@ Deployment and upgrade related functions come with an optional `opts` object, wh * `delegatecall` * `selfdestruct` * `missing-public-upgradeto` - * `struct-internal-function` + * `internal-function-storage` This function is a generalized version of the original `unsafeAllowCustomTypes` and `unsafeAllowLinkedLibraries` allowing any check to be manually disabled. diff --git a/packages/core/contracts/test/Storage.sol b/packages/core/contracts/test/Storage.sol index b40d85f5e..446c8913c 100644 --- a/packages/core/contracts/test/Storage.sol +++ b/packages/core/contracts/test/Storage.sol @@ -906,8 +906,8 @@ contract StorageUpgrade_FunctionPointer_V1 { uint256 a; function(bool) external b; } - S private s; + function(bool) external c; } contract StorageUpgrade_FunctionPointer_V2_Ok { @@ -915,8 +915,8 @@ contract StorageUpgrade_FunctionPointer_V2_Ok { uint256 a; function(bool) external b; } - S private s; + function(bool) external c; } contract StorageUpgrade_FunctionPointer_V2_Bad { @@ -924,6 +924,6 @@ contract StorageUpgrade_FunctionPointer_V2_Bad { uint256 a; function(bool) b; } - S private s; + function(bool) c; } \ No newline at end of file diff --git a/packages/core/contracts/test/Validations.sol b/packages/core/contracts/test/Validations.sol index 507995bd4..e1915997b 100644 --- a/packages/core/contracts/test/Validations.sol +++ b/packages/core/contracts/test/Validations.sol @@ -207,20 +207,32 @@ contract TransitiveLibraryIsUnsafe { } } -contract ExternalFunctionPointer { +contract StructExternalFunctionPointer { struct S { function(bool) external foo; } } -contract InternalFunctionPointer { +contract StructInternalFunctionPointer { struct S { function(bool) internal foo; } } -contract ImpliedInternalFunctionPointer { +contract StructImpliedInternalFunctionPointer { struct S { function(bool) foo; } -} \ No newline at end of file +} + +contract ExternalFunctionPointer { + function(bool) external foo; +} + +contract InternalFunctionPointer { + function(bool) internal foo; +} + +contract ImpliedInternalFunctionPointer { + function(bool) foo; +} diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md index 265117f3b..86aa1f1ab 100644 --- a/packages/core/src/cli/cli.test.ts.md +++ b/packages/core/src/cli/cli.test.ts.md @@ -19,7 +19,7 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function␊ + --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` @@ -39,7 +39,7 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, struct-internal-function␊ + --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index 07907c06b30441de92d5f57eed76ffe88aa60cb5..4cc73afe1253c420a04aa596b631e63f24d7fd4d 100644 GIT binary patch literal 2148 zcmV-q2%GmoRzVyn78hL?WUm*8A>?7oy>Yg9=@JF7J9Lo!k zfNgT7yMDg@hR5>Nzezv?M(iax4P*&mV0Y2l(0bfBa$X`kMNz zUt9a*d!JwLOFSYq#JyAWPm2U3x;xL|Qs;|j0PcPHlo}S$fiE5 z7yblcZl`J_dO0S+nD_|(f?HrgED%ZTkd0EQ?vFy3@Icl|2+i7rP{#|X;Fuu~2V)cn zC1s*= z3MJTw(J+(&025(CICWwN%h3*5!3eE^G4W?x!@j}~;PWzx+ZyW%k z2LK?)E>TuHA!8HtB7|r;l-TigM}wSh1~8IvY6rMT%z-Usgc(~x63Wks7KLqtDIe_( zZLv=r@PiKt+U_>@n%!o*(L6MaZ(xoD2b*ClU}r0OMOqARg~k9;K$c0Nus)$SXvppQ z)Cv;>(5Iscq4FWu5W^6iK=TRvR08UY;L#CQLLWv!u8A`%P01*YIBda@$qocq8EzAY za$+YCPR&rfgZl$Q`Dh-z7a;)LO|8EqTx}717f=;goko@wa+SHgW9s!HAR1#bJl3uo zZpXGrL}-%!nefI+0ah?AQK{lGOT95ZA&?Om&ti1oV}Anw@?#LE_W0p2 zurxcUG7uK?(9K)`@SG(R?8TMcmiF8A8eH3$~)pqbw=eY>U?!bnp{fX){-lV#oAKJb`tM(@{;jQ4zj@4DT>qWC z@oI4>FwhsR;`aX)xA;Jidr-#v{V#gxW+E#ZFNz#fxAKPj-ewK8TwsYR!Z>Ms;!+4_ zP*e=jQJ6@+iESGSgS>U5%KY?to!bonC%Ii7Oh6$lN=l}ISWHl)oa`H&G87@ohb*+y z_G2JaN1?k2)ojM1bSJIHhQvEVcy`!J-M5UEyaN5z>7)_{l*!7B5ek$7!M&fi?wG+n z@G+OrIRWAqh_O-hZsn#{)S_@ZZYTD2v(VHDWh^N)u9D8u@cSxyxnjx9@lg3MR zKDO?ejt&n;*gFExp|jh(`{|ck51u1y2N^$arRG?u$Aqz8VN_LI-AtG4KnoopEfhw2 zA5YY62@yhDxe4-!KVS$yBzw^3CPl3A5w>H93aT|MPN`sAt3jv2h#C%#PLB+z zJ}1P}V?o;ck`tmwy^yO0G_UzRNM9DYT-5Tm;6}>l=5e+9;CHDFIoOfZ^!h;Rb%y!5 z!wqL?Jsl3^#gHqW{MD`7SC#$MJNMp!^xvzu_M5xtW_4@HV{LA(@7n-T6)#_@fy4+B;jAwtz#j^!Dd^ba* zPu?<(=Fj%@%7xqHXyG~HqQkgQ=e-Q}?Od>z7rsPV@RWEJjY~zNVMJr@9hU`%yBQL* z2~rOqR0N@l%(sf)m@!#Dd;3#suit1id%fg!!)f!9ZbmOr<{kZChJ3#i?1|smsd*V& zw$6u90p}aPm!S4+y_13MVZoeDQeyKEkUMOP=_L9#M$mU>4xEqAX0iAz=JcyfaQ?NH z^L1Mql+N)Ri^*&ne`xpzIcu~G^KMi#XV9P9;x!|4`fk!Sl!)Vb!Cz+t`%B@OLZJPc z{!IOXL4Ki8#u#5s$N1an7@wf=r{$G&iRn)=rf*-u^wOdUX|`lTV=ff~ph3j$fQ~}- zt~fPO74&%%|CTYavCzb)>3<@CbIv7g_@mCdc&v#o%v<_6lN@mt$Twp2_nDOWc5%w& avHCU1vOq*s@-jk<#Q6=B~ literal 2142 zcmV-k2%+~uRzVa;;F$<@dqhrMK9V_zWH7s!1Nd4!y|y61;I{E=rQ$MOOs zV4IxjuAi^I>Z+RY&(45};jw)6FA~sz5qnISMp8ibVDh;m0DoBFU` z_!ES=ovM-O<(LFx;v@JAZh-}{KqRq4HcF+sKMGyK16eB}G;0$=9WS7QV}?8&j8Pzz zoW*Kij_Gi$O~jN-^rY8mBZo3&0)?$T0T$6<0xMh(F8i?G3j=Mf2m|=Kfp&?5Lk8}F zo*UQ?sH2>%XmyrScLO=XM~|^@k8yxJUwor6f@?QG#@xa^5po+$OBQaRmV+iD^ph(n zlwco5!%zkQXk^FKr?Og%T)2s(M~2jyAPVXhx(^sDI3v6?n3k2q3vD+#?SpQ;aR7)O z0Dv63L|N^Gj7`vs5TfN!V#n7V4RX2}z(~TW9pEA{2eyTDwMFb*KviIM8d+AzRp$1Nsn?5uXpG75Si5ex z9or%ip-K8@!W%0ESi!VJrHace^~U&wK!)T%F9IPlug;iwi9X(bTHkN&*43eQ*z9&Y z-QLc}2=xd97Sa;}z}Qr3)(Wm!;TmV8vXA(_@FA`Z;kww81R|~QMvOvEYCz15F?}4N zv_pVuz!g|-od7?S77I&BM_e6u<=I=*b;Ce?DK}H**2NiU}E2ozkAkqkxmWyuHh zq)Z*{Nwf~eKxV~fWMe)ez6fPPIG11^#I(~Hfz>jGA)LTg;RhR~xbTiRSnZGGs`jpG z@2d8$YVWG{u4?b9_P(9k`ywrsRxEhF3V%3Lg+J^CHkegI=)eE`5lxT63RJ?NGGw&! zG&xD7V6`52T`J{U`xwiX3Nc(eBYuHh{V1*nLht#h*q+9eOnn_B5TZY?6LRNHO|c?M zNBRX{-zRQ4M+gLXa0#DW|l{1fR7+p_Aht9fvyPDVi z&!GdL9wWv4y$k;Kr9J}8oKq}R1wqrF6tQ}s(8QuMGxIfWJS|Okz?vs-f-XBtf7_*EKxNWCw)&`3E>QiY9Trb z6UjHRZ9`#@w~kbmpI)zXy8+-Nx66YGD1=2usZ2gGL6LH@Z#2qKgeV`f&`!ILflv*F zZX#5#8H>`5w4NFg?+oGDSub_pGCJ}Kv{$E-N*GWkD>FtYPznV1e%`uc2KT_nTtedn zh+iPaM$x;Kn_5we!tJ=7*w@WMQy1}~Cd4A1R&#Ug!M#5qrSfTpWZq92FV*?jx@S5% zJRD)~2t0?5Zu9P^UvE8lj;tMI{JfQ#W1$`s#(srSRdIDQU9tlmbbz!_nCN{xQMV;T z2yNvi$Rh%+8iT~&0R4Nt7(lDKdgX~V&xy~@sQ=&=m* zy?mI1>M-EI8cFkeE%7dibCs z2vuahRs7zJ$@&dK0cep;`;d^H{8Z>M8?g2rE#SJEY>KgpQBeF@V`izcMmk`0ZyR1AOy5xWCA3e~&f)I?R# z=S}=FV`5{WiBHr2LjdQTOWN>9opIR;w+GF#OR+gDf7ePl*wcDYm#Mw Uh^XXcgcynQKLE*lMr|$t0AtVwrT_o{ diff --git a/packages/core/src/storage.test.ts b/packages/core/src/storage.test.ts index 08c7aa411..f9af1a667 100644 --- a/packages/core/src/storage.test.ts +++ b/packages/core/src/storage.test.ts @@ -946,7 +946,7 @@ test('storage upgrade with function pointers', t => { t.deepEqual(getStorageUpgradeErrors(v1, v2_Ok), []); t.like(getStorageUpgradeErrors(v1, v2_Bad), { - length: 1, + length: 2, 0: { kind: 'typechange', change: { @@ -964,5 +964,13 @@ test('storage upgrade with function pointers', t => { original: { label: 's' }, updated: { label: 's' }, }, + 1: { + kind: 'typechange', + change: { + kind: 'visibility change', + }, + original: { label: 'c' }, + updated: { label: 'c' }, + } }); }); diff --git a/packages/core/src/validate.test.ts b/packages/core/src/validate.test.ts index 50e75de68..4f002ad84 100644 --- a/packages/core/src/validate.test.ts +++ b/packages/core/src/validate.test.ts @@ -140,10 +140,15 @@ testValid('TransitiveLibraryIsUnsafe', 'transparent', false); testValid('contracts/test/ValidationsSameNameSafe.sol:SameName', 'transparent', true); testValid('contracts/test/ValidationsSameNameUnsafe.sol:SameName', 'transparent', false); +testValid('StructExternalFunctionPointer', 'transparent', true); +testValid('StructInternalFunctionPointer', 'transparent', false); +testValid('StructImpliedInternalFunctionPointer', 'transparent', false); +testOverride('StructImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['internal-function-storage'] }, true); + testValid('ExternalFunctionPointer', 'transparent', true); testValid('InternalFunctionPointer', 'transparent', false); testValid('ImpliedInternalFunctionPointer', 'transparent', false); -testOverride('ImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['struct-internal-function'] }, true); +testOverride('ImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['internal-function-storage'] }, true); test('ambiguous name', t => { const error = t.throws(() => getContractVersion(t.context.validation, 'SameName')); diff --git a/packages/core/src/validate/overrides.ts b/packages/core/src/validate/overrides.ts index 7affb2028..b4ebbb904 100644 --- a/packages/core/src/validate/overrides.ts +++ b/packages/core/src/validate/overrides.ts @@ -76,10 +76,10 @@ export const ValidationErrorUnsafeMessages: Record = { ` @openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol`, link: 'https://zpl.in/upgrades/error-008', }, - 'struct-internal-function': { + 'internal-function-storage': { msg: e => `Struct has internal function \`${e.name}\``, hint: () => `Use external functions to avoid code pointers that will no longer be valid after an upgrade\n` + - ` or skip this check with the \`unsafeAllow.struct-internal-function\` flag and\n` + - ` ensure you always reassign the internal functions in structs during upgrades`, + ` or skip this check with the \`unsafeAllow.internal-function-storage\` flag and\n` + + ` ensure you always reassign the internal functions in storage variables during upgrades`, }, }; diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index c711fb5b0..cd49a9644 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -40,7 +40,7 @@ export const errorKinds = [ 'delegatecall', 'selfdestruct', 'missing-public-upgradeto', - 'struct-internal-function', + 'internal-function-storage', ] as const; export type ValidationError = @@ -62,7 +62,7 @@ interface ValidationErrorWithName extends ValidationErrorBase { | 'external-library-linking' | 'struct-definition' | 'enum-definition' - | 'struct-internal-function'; + | 'internal-function-storage'; } interface ValidationErrorConstructor extends ValidationErrorBase { @@ -199,7 +199,7 @@ export function validate( // TODO: add linked libraries support // https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/52 ...getLinkingErrors(contractDef, bytecode), - ...getStructFunctionErrors(contractDef, decodeSrc), + ...getInternalFunctionStorageErrors(contractDef, decodeSrc), ]; validation[key].layout = extractStorageLayout( @@ -561,13 +561,24 @@ function* getLinkingErrors( } } -function* getStructFunctionErrors(contractDef: ContractDefinition, decodeSrc: SrcDecoder): Generator { - // Note: Solidity currently does not include struct annotations in the AST, so this check cannot be skipped with annotations +function* getInternalFunctionStorageErrors(contractDef: ContractDefinition, decodeSrc: SrcDecoder): Generator { + // Note: Solidity does not allow annotations for non-public state variables, so this cannot be skipped with annotations + for (const variableDef of findAll('VariableDeclaration', contractDef)) { + if (variableDef.typeName?.nodeType === 'FunctionTypeName' && variableDef.typeName.visibility === 'internal') { + yield { + kind: 'internal-function-storage', + name: variableDef.name, + src: decodeSrc(variableDef), + }; + } + } + + // Note: Solidity currently does not include struct annotations in the AST, so this cannot be skipped with annotations for (const structDef of findAll('StructDefinition', contractDef)) { for (const member of structDef.members) { if (member.typeName?.nodeType === 'FunctionTypeName' && member.typeName.visibility === 'internal') { yield { - kind: 'struct-internal-function', + kind: 'internal-function-storage', name: member.name, src: decodeSrc(member), }; From 4e0054acfb3ef0954b7a15358f36435729ff4051 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 7 Jun 2024 18:13:44 -0400 Subject: [PATCH 09/22] Fix lint --- packages/core/src/storage.test.ts | 2 +- packages/core/src/validate.test.ts | 7 ++++++- packages/core/src/validate/run.ts | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/core/src/storage.test.ts b/packages/core/src/storage.test.ts index f9af1a667..89113aaea 100644 --- a/packages/core/src/storage.test.ts +++ b/packages/core/src/storage.test.ts @@ -971,6 +971,6 @@ test('storage upgrade with function pointers', t => { }, original: { label: 'c' }, updated: { label: 'c' }, - } + }, }); }); diff --git a/packages/core/src/validate.test.ts b/packages/core/src/validate.test.ts index 4f002ad84..bd7eb5b95 100644 --- a/packages/core/src/validate.test.ts +++ b/packages/core/src/validate.test.ts @@ -143,7 +143,12 @@ testValid('contracts/test/ValidationsSameNameUnsafe.sol:SameName', 'transparent' testValid('StructExternalFunctionPointer', 'transparent', true); testValid('StructInternalFunctionPointer', 'transparent', false); testValid('StructImpliedInternalFunctionPointer', 'transparent', false); -testOverride('StructImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['internal-function-storage'] }, true); +testOverride( + 'StructImpliedInternalFunctionPointer', + 'transparent', + { unsafeAllow: ['internal-function-storage'] }, + true, +); testValid('ExternalFunctionPointer', 'transparent', true); testValid('InternalFunctionPointer', 'transparent', false); diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index cd49a9644..1c5573f64 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -561,7 +561,10 @@ function* getLinkingErrors( } } -function* getInternalFunctionStorageErrors(contractDef: ContractDefinition, decodeSrc: SrcDecoder): Generator { +function* getInternalFunctionStorageErrors( + contractDef: ContractDefinition, + decodeSrc: SrcDecoder, +): Generator { // Note: Solidity does not allow annotations for non-public state variables, so this cannot be skipped with annotations for (const variableDef of findAll('VariableDeclaration', contractDef)) { if (variableDef.typeName?.nodeType === 'FunctionTypeName' && variableDef.typeName.visibility === 'internal') { From a54394df1d5051ab72b3c91c577827656e98d6cd Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 7 Jun 2024 18:15:02 -0400 Subject: [PATCH 10/22] Update changelog --- packages/core/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index ab4effb80..187e4bc58 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Fix storage layout comparison for function types in structs, disallow internal functions in structs. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) +- Fix storage layout comparison for function types, disallow internal functions in storage. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) ## 1.33.1 (2024-04-25) From bb3dc314a76e8761bfb8c27db49b961e22c75354 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 7 Jun 2024 23:49:20 -0400 Subject: [PATCH 11/22] Remove redundant loop --- packages/core/src/validate/run.ts | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 1c5573f64..8a923642a 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -566,26 +566,13 @@ function* getInternalFunctionStorageErrors( decodeSrc: SrcDecoder, ): Generator { // Note: Solidity does not allow annotations for non-public state variables, so this cannot be skipped with annotations - for (const variableDef of findAll('VariableDeclaration', contractDef)) { - if (variableDef.typeName?.nodeType === 'FunctionTypeName' && variableDef.typeName.visibility === 'internal') { + for (const variableDec of findAll('VariableDeclaration', contractDef)) { + if (variableDec.typeName?.nodeType === 'FunctionTypeName' && variableDec.typeName.visibility === 'internal') { yield { kind: 'internal-function-storage', - name: variableDef.name, - src: decodeSrc(variableDef), + name: variableDec.name, + src: decodeSrc(variableDec), }; } } - - // Note: Solidity currently does not include struct annotations in the AST, so this cannot be skipped with annotations - for (const structDef of findAll('StructDefinition', contractDef)) { - for (const member of structDef.members) { - if (member.typeName?.nodeType === 'FunctionTypeName' && member.typeName.visibility === 'internal') { - yield { - kind: 'internal-function-storage', - name: member.name, - src: decodeSrc(member), - }; - } - } - } } From 9a7bd0030bb5dd181dc34d5b6e5b99b0f064a059 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 10 Jun 2024 14:49:51 -0400 Subject: [PATCH 12/22] Recursively handle struct variables --- packages/core/contracts/test/Validations.sol | 18 ++++++++++++++ packages/core/src/validate.test.ts | 4 +++ packages/core/src/validate/run.ts | 26 +++++++++++++++++--- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/packages/core/contracts/test/Validations.sol b/packages/core/contracts/test/Validations.sol index e1915997b..4359aa952 100644 --- a/packages/core/contracts/test/Validations.sol +++ b/packages/core/contracts/test/Validations.sol @@ -225,6 +225,24 @@ contract StructImpliedInternalFunctionPointer { } } +struct StandaloneStructInternalFn { + function(bool) internal foo; +} + +contract UsesStandaloneStructInternalFn { + StandaloneStructInternalFn s; +} + +contract StructUsesStandaloneStructInternalFn { + struct S2 { + StandaloneStructInternalFn s; + } +} + +contract RecursiveStructUsesStandaloneStructInternalFn { + StructUsesStandaloneStructInternalFn.S2 s2; +} + contract ExternalFunctionPointer { function(bool) external foo; } diff --git a/packages/core/src/validate.test.ts b/packages/core/src/validate.test.ts index bd7eb5b95..5a67d3973 100644 --- a/packages/core/src/validate.test.ts +++ b/packages/core/src/validate.test.ts @@ -150,6 +150,10 @@ testOverride( true, ); +testValid('UsesStandaloneStructInternalFn', 'transparent', false); +testValid('StructUsesStandaloneStructInternalFn', 'transparent', false); +testValid('RecursiveStructUsesStandaloneStructInternalFn', 'transparent', false); + testValid('ExternalFunctionPointer', 'transparent', true); testValid('InternalFunctionPointer', 'transparent', false); testValid('ImpliedInternalFunctionPointer', 'transparent', false); diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 8a923642a..7192027a0 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -1,6 +1,6 @@ import { Node } from 'solidity-ast/node'; import { isNodeType, findAll, ASTDereferencer, astDereferencer } from 'solidity-ast/utils'; -import type { ContractDefinition, FunctionDefinition } from 'solidity-ast'; +import type { ContractDefinition, FunctionDefinition, StructDefinition } from 'solidity-ast'; import debug from '../utils/debug'; import { SolcOutput, SolcBytecode, SolcInput } from '../solc-api'; @@ -199,7 +199,7 @@ export function validate( // TODO: add linked libraries support // https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/52 ...getLinkingErrors(contractDef, bytecode), - ...getInternalFunctionStorageErrors(contractDef, decodeSrc), + ...getInternalFunctionStorageErrors(contractDef, deref, decodeSrc), ]; validation[key].layout = extractStorageLayout( @@ -458,6 +458,16 @@ function tryDerefFunction(deref: ASTDereferencer, referencedDeclaration: number) } } +function tryDerefStruct(deref: ASTDereferencer, referencedDeclaration: number): StructDefinition | undefined { + try { + return deref(['StructDefinition'], referencedDeclaration); + } catch (e: any) { + if (!e.message.includes('No node with id')) { + throw e; + } + } +} + function* getInheritedContractOpcodeErrors( contractDef: ContractDefinition, deref: ASTDereferencer, @@ -562,17 +572,25 @@ function* getLinkingErrors( } function* getInternalFunctionStorageErrors( - contractDef: ContractDefinition, + contractOrStructDef: ContractDefinition | StructDefinition, + deref: ASTDereferencer, decodeSrc: SrcDecoder, ): Generator { // Note: Solidity does not allow annotations for non-public state variables, so this cannot be skipped with annotations - for (const variableDec of findAll('VariableDeclaration', contractDef)) { + for (const variableDec of findAll('VariableDeclaration', contractOrStructDef)) { if (variableDec.typeName?.nodeType === 'FunctionTypeName' && variableDec.typeName.visibility === 'internal') { + // Find internal function types directly in this node's scope yield { kind: 'internal-function-storage', name: variableDec.name, src: decodeSrc(variableDec), }; + } else if (variableDec.typeName?.nodeType === 'UserDefinedTypeName') { + // Recursively dereference structs, since structs may be declared elsewhere + const structDef = tryDerefStruct(deref, variableDec.typeName.referencedDeclaration); + if (structDef !== undefined) { + yield* getInternalFunctionStorageErrors(structDef, deref, decodeSrc); + } } } } From ab9e720fe5e1e5320ecaec9132ad0b9ae8eb6d05 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 10 Jun 2024 15:11:42 -0400 Subject: [PATCH 13/22] Update comment --- packages/core/src/validate/run.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 7192027a0..8f0e22aa7 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -576,7 +576,8 @@ function* getInternalFunctionStorageErrors( deref: ASTDereferencer, decodeSrc: SrcDecoder, ): Generator { - // Note: Solidity does not allow annotations for non-public state variables, so this cannot be skipped with annotations + // Note: Solidity does not allow annotations for non-public state variables, nor recursive types for public variables, + // so annotations cannot be used to skip these checks. for (const variableDec of findAll('VariableDeclaration', contractOrStructDef)) { if (variableDec.typeName?.nodeType === 'FunctionTypeName' && variableDec.typeName.visibility === 'internal') { // Find internal function types directly in this node's scope From 406253a66e4dfc9dcd90a1136f2bc0f6d70ebb8f Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 10 Jun 2024 16:51:37 -0400 Subject: [PATCH 14/22] Add documentation section on internal functions --- docs/modules/ROOT/pages/faq.adoc | 81 ++++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 9 deletions(-) diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index 890c82474..298b7061f 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -22,9 +22,7 @@ As a replacement for the constructor, it is common to set up an `initialize` fun [source,solidity] ---- -import "@openzeppelin/contracts/proxy/utils/Initializable.sol"; -// Alternatively, if you are using @openzeppelin/contracts-upgradeable: -// import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; contract MyContract is Initializable { uint256 value; @@ -194,13 +192,14 @@ When using UUPS proxies (through the `kind: 'uups'` option), the implementation The recommended way to include one or both of these functions is by inheriting the `UUPSUpgradeable` contract provided in OpenZeppelin Contracts, as shown below. This contract adds the required function(s), but also contains a built-in mechanism that will check on-chain, at the time of an upgrade, that the new implementation proposed also inherits `UUPSUpgradeable` or implements the same interface. In this way, when using the Upgrades Plugins there are two layers of mitigations to prevent accidentally disabling upgradeability: an off-chain check by the plugins, and an on-chain fallback in the contract itself. -```solidity +[source,solidity] +---- import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; contract MyContract is Initializable, ..., UUPSUpgradeable { ... } -``` +---- Read more about the differences with the Transparent Proxy Pattern in xref:contracts:api:proxy.adoc#transparent-vs-uups[Transparent vs UUPS]. @@ -218,7 +217,8 @@ Renaming a variable is disallowed by default because there is a chance that a re It is possible to disable this check by passing the option `unsafeAllowRenames: true`. A more granular approach is to use a docstring comment `/// @custom:oz-renamed-from ` right above the variable that is being renamed, for example: -``` +[source,solidity] +---- contract V1 { uint x; } @@ -226,11 +226,12 @@ contract V2 { /// @custom:oz-renamed-from x uint y; } -``` +---- Changing the type of a variable is not allowed either, even in cases where the types have the same size and alignment, for the similar reason explained above. As long as we can guarantee that the rest of the layout is not affected by this type change, it is also possible to override this check by placing a docstring comment `/// @custom:oz-retyped-from `. -``` +[source,solidity] +---- contract V1 { bool x; } @@ -238,6 +239,68 @@ contract V2 { /// @custom:oz-retyped-from bool uint8 x; } -``` +---- Docstring comments don't yet work for struct members, due to a current Solidity limitation. + +[[internal-function-storage]] +== How can I use internal functions in storage variables? + +Internal functions in storage variables are code pointers which will no longer be valid after an upgrade, because the code will move around and the pointer would change. To avoid this issue, you can either declare those functions as external, or reassign internal functions during upgrades. + +For example, the `messageFunction` variable in the following contract is not upgrade safe. Attempting to call `showMessage()` after an upgrade would likely result in a revert. +[source,solidity] +---- +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; + +contract V1 is Initializable { + function() internal pure returns (string memory) messageFunction; + + function initialize() initializer public { + messageFunction = hello; + } + + function hello() internal pure returns (string memory) { + return "Hello, World!"; + } + + function showMessage() public view returns (string memory) { + return messageFunction(); + } + ... +} +---- + +To allow the above contract to be deployed by the Upgrades Plugins, you can disable the `internal-function-storage` check according to <>, but ensure you follow the steps below to reassign the internal function during upgrades. + +In new versions of this contract, assign the internal function in the storage variable again, for example by using a reinitializer function: +[source,solidity] +---- +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import "./V1.sol"; + +contract V2 is V1 { + function initializeV2() reinitializer(2) public { + messageFunction = hello; + } + ... +} +---- + +Then when upgrading, call the reinitializer function as part of the upgrade process, for example in Hardhat: +[source,ts] +---- +await upgrades.upgradeProxy(PROXY_ADDRESS, ContractFactoryV2, { + call: 'initializeV2', + unsafeAllow: ['internal-function-storage'] +}); +---- +or in Foundry: +[source,solidity] +---- +Upgrades.upgradeProxy( + PROXY_ADDRESS, + "V2.sol", + abi.encodeCall(V2.initializeV2, ()) +); +---- From 14062bffa64c7cfa62ed8cdd608a12d223b8b75c Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 10 Jun 2024 16:54:28 -0400 Subject: [PATCH 15/22] add link --- docs/modules/ROOT/pages/faq.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index 298b7061f..bac2a5eac 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -273,7 +273,7 @@ contract V1 is Initializable { To allow the above contract to be deployed by the Upgrades Plugins, you can disable the `internal-function-storage` check according to <>, but ensure you follow the steps below to reassign the internal function during upgrades. -In new versions of this contract, assign the internal function in the storage variable again, for example by using a reinitializer function: +In new versions of this contract, assign the internal function in the storage variable again, for example by using a https://docs.openzeppelin.com/contracts/api/proxy#Initializable-reinitializer-uint64-[reinitializer]: [source,solidity] ---- import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; From 58daa4168bc325c4f80302039dc14d313fb18668 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 10 Jun 2024 17:20:38 -0400 Subject: [PATCH 16/22] Improve docs and errors --- docs/modules/ROOT/pages/api-hardhat-upgrades.adoc | 2 +- packages/core/src/validate/overrides.ts | 2 +- packages/core/src/validate/report.ts | 9 +++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 09d67693a..1b951d5cd 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -16,7 +16,7 @@ The following options are common to some functions. ** `"constructor"`: Allows defining a constructor. See `constructorArgs`. ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?] ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` or `upgradeToAndCall` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism. -** `"internal-function-storage"`: Allow internal functions in storage variables. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. +** `"internal-function-storage"`: Allow internal functions in storage variables. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. See xref:faq.adoc#internal-function-storage[How can I use internal functions in storage variables?] * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming. * `unsafeSkipStorageCheck`: (`boolean`) upgrades the proxy or beacon without first checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. * `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables. diff --git a/packages/core/src/validate/overrides.ts b/packages/core/src/validate/overrides.ts index b4ebbb904..3c0d1a508 100644 --- a/packages/core/src/validate/overrides.ts +++ b/packages/core/src/validate/overrides.ts @@ -79,7 +79,7 @@ export const ValidationErrorUnsafeMessages: Record = { link: 'https://zpl.in/upgrades/error-008', }, 'internal-function-storage': { - msg: e => `Struct has internal function \`${e.name}\``, + msg: e => `Variable \`${e.name}\` is an internal function`, hint: () => - `Use external functions to avoid code pointers that will no longer be valid after an upgrade\n` + - ` or skip this check with the \`unsafeAllow.internal-function-storage\` flag and\n` + - ` ensure you always reassign the internal functions in storage variables during upgrades`, + `Use external functions in storage variables, or skip this check with the\n` + + ` \`unsafeAllow.internal-function-storage\` flag and ensure you always\n` + + ` reassign internal functions in storage during upgrades`, + link: 'https://zpl.in/upgrades/error-009', }, }; From 7985d71c62bd6b794b1cd4046448e694674fcc37 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Tue, 11 Jun 2024 14:16:24 -0400 Subject: [PATCH 17/22] Update docs/modules/ROOT/pages/faq.adoc Co-authored-by: Francisco --- docs/modules/ROOT/pages/faq.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index bac2a5eac..4dffdc57d 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -246,7 +246,7 @@ Docstring comments don't yet work for struct members, due to a current Solidity [[internal-function-storage]] == How can I use internal functions in storage variables? -Internal functions in storage variables are code pointers which will no longer be valid after an upgrade, because the code will move around and the pointer would change. To avoid this issue, you can either declare those functions as external, or reassign internal functions during upgrades. +Internal functions in storage variables are code pointers which will no longer be valid after an upgrade, because the code will move around and the pointer would change. To avoid this issue, you can either declare those functions as external, or reassign internal functions during upgrades. Alternatively, avoid code pointers in storage altogether, and define an `enum` that you will use with a dispatcher function to select from the list of available functions. For example, the `messageFunction` variable in the following contract is not upgrade safe. Attempting to call `showMessage()` after an upgrade would likely result in a revert. [source,solidity] From e9120163dcd03de4e2005c2880c8b4ee1764c7ee Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Tue, 11 Jun 2024 14:32:15 -0400 Subject: [PATCH 18/22] Update wording --- docs/modules/ROOT/pages/faq.adoc | 2 +- packages/core/src/validate/report.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc index 4dffdc57d..027912919 100644 --- a/docs/modules/ROOT/pages/faq.adoc +++ b/docs/modules/ROOT/pages/faq.adoc @@ -246,7 +246,7 @@ Docstring comments don't yet work for struct members, due to a current Solidity [[internal-function-storage]] == How can I use internal functions in storage variables? -Internal functions in storage variables are code pointers which will no longer be valid after an upgrade, because the code will move around and the pointer would change. To avoid this issue, you can either declare those functions as external, or reassign internal functions during upgrades. Alternatively, avoid code pointers in storage altogether, and define an `enum` that you will use with a dispatcher function to select from the list of available functions. +Internal functions in storage variables are code pointers which will no longer be valid after an upgrade, because the code will move around and the pointer would change. To avoid this issue, you can declare those functions as external, or avoid code pointers in storage altogether and define an `enum` that you will use with a dispatcher function to select from the list of available functions. If you must use internal functions, those internal functions need to be reassigned during each upgrade. For example, the `messageFunction` variable in the following contract is not upgrade safe. Attempting to call `showMessage()` after an upgrade would likely result in a revert. [source,solidity] diff --git a/packages/core/src/validate/report.ts b/packages/core/src/validate/report.ts index 6e2062d1b..44b42cf9a 100644 --- a/packages/core/src/validate/report.ts +++ b/packages/core/src/validate/report.ts @@ -67,9 +67,9 @@ const errorInfo: ErrorDescriptions = { 'internal-function-storage': { msg: e => `Variable \`${e.name}\` is an internal function`, hint: () => - `Use external functions in storage variables, or skip this check with the\n` + - ` \`unsafeAllow.internal-function-storage\` flag and ensure you always\n` + - ` reassign internal functions in storage during upgrades`, + `Use external functions or avoid functions in storage.\n` + + ` If you must use internal functions, skip this check with the \`unsafeAllow.internal-function-storage\`\n` + + ` flag and ensure you always reassign internal functions in storage during upgrades`, link: 'https://zpl.in/upgrades/error-009', }, }; From e05f9663f5ada7f2244b7011f91b98c97edc5283 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 12 Jun 2024 10:07:47 -0400 Subject: [PATCH 19/22] Recursively traverse array or mapping types --- packages/core/contracts/test/Validations.sol | 32 +++++++++++++--- packages/core/src/validate.test.ts | 6 ++- packages/core/src/validate/run.ts | 40 +++++++++++++++++--- 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/packages/core/contracts/test/Validations.sol b/packages/core/contracts/test/Validations.sol index 4359aa952..5e9856497 100644 --- a/packages/core/contracts/test/Validations.sol +++ b/packages/core/contracts/test/Validations.sol @@ -230,17 +230,39 @@ struct StandaloneStructInternalFn { } contract UsesStandaloneStructInternalFn { - StandaloneStructInternalFn s; + StandaloneStructInternalFn bad; } contract StructUsesStandaloneStructInternalFn { - struct S2 { - StandaloneStructInternalFn s; + struct Bad { + StandaloneStructInternalFn bad; } } -contract RecursiveStructUsesStandaloneStructInternalFn { - StructUsesStandaloneStructInternalFn.S2 s2; +contract RecursiveStructInternalFn { + StructUsesStandaloneStructInternalFn.Bad bad; +} + +contract MappingRecursiveStructInternalFn { + mapping(address => mapping(address => StructUsesStandaloneStructInternalFn.Bad)) bad; +} + +contract ArrayRecursiveStructInternalFn { + StructUsesStandaloneStructInternalFn.Bad[][] bad; +} + +contract SelfRecursiveMappingStructInternalFn { + struct SelfRecursive { + mapping(address => SelfRecursive) selfReference; + mapping(address => StructUsesStandaloneStructInternalFn.Bad) bad; + } +} + +contract SelfRecursiveArrayStructInternalFn { + struct SelfRecursiveArray { + SelfRecursiveArray[] selfReference; + StructUsesStandaloneStructInternalFn.Bad[] bad; + } } contract ExternalFunctionPointer { diff --git a/packages/core/src/validate.test.ts b/packages/core/src/validate.test.ts index 5a67d3973..8be4e7bf4 100644 --- a/packages/core/src/validate.test.ts +++ b/packages/core/src/validate.test.ts @@ -152,7 +152,11 @@ testOverride( testValid('UsesStandaloneStructInternalFn', 'transparent', false); testValid('StructUsesStandaloneStructInternalFn', 'transparent', false); -testValid('RecursiveStructUsesStandaloneStructInternalFn', 'transparent', false); +testValid('RecursiveStructInternalFn', 'transparent', false); +testValid('MappingRecursiveStructInternalFn', 'transparent', false); +testValid('ArrayRecursiveStructInternalFn', 'transparent', false); +testValid('SelfRecursiveMappingStructInternalFn', 'transparent', false); +testValid('SelfRecursiveArrayStructInternalFn', 'transparent', false); testValid('ExternalFunctionPointer', 'transparent', true); testValid('InternalFunctionPointer', 'transparent', false); diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 8f0e22aa7..214361df2 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -1,6 +1,12 @@ import { Node } from 'solidity-ast/node'; import { isNodeType, findAll, ASTDereferencer, astDereferencer } from 'solidity-ast/utils'; -import type { ContractDefinition, FunctionDefinition, StructDefinition } from 'solidity-ast'; +import type { + ContractDefinition, + FunctionDefinition, + StructDefinition, + TypeName, + UserDefinedTypeName, +} from 'solidity-ast'; import debug from '../utils/debug'; import { SolcOutput, SolcBytecode, SolcInput } from '../solc-api'; @@ -575,6 +581,7 @@ function* getInternalFunctionStorageErrors( contractOrStructDef: ContractDefinition | StructDefinition, deref: ASTDereferencer, decodeSrc: SrcDecoder, + visitedNodeIds = new Set(), ): Generator { // Note: Solidity does not allow annotations for non-public state variables, nor recursive types for public variables, // so annotations cannot be used to skip these checks. @@ -586,12 +593,33 @@ function* getInternalFunctionStorageErrors( name: variableDec.name, src: decodeSrc(variableDec), }; - } else if (variableDec.typeName?.nodeType === 'UserDefinedTypeName') { - // Recursively dereference structs, since structs may be declared elsewhere - const structDef = tryDerefStruct(deref, variableDec.typeName.referencedDeclaration); - if (structDef !== undefined) { - yield* getInternalFunctionStorageErrors(structDef, deref, decodeSrc); + } else if (variableDec.typeName) { + const userDefinedType = findUserDefinedType(variableDec.typeName); + // Recursively try to dereference struct since it may be declared elsewhere + if (userDefinedType !== undefined && !visitedNodeIds.has(userDefinedType.referencedDeclaration)) { + const structDef = tryDerefStruct(deref, userDefinedType.referencedDeclaration); + if (structDef !== undefined) { + visitedNodeIds.add(userDefinedType.referencedDeclaration); + yield* getInternalFunctionStorageErrors(structDef, deref, decodeSrc, visitedNodeIds); + } } } } } + +/** + * Recursively traverse array and mapping types to find user-defined types (which may be struct references). + */ +function findUserDefinedType(typeName: TypeName): UserDefinedTypeName | undefined { + switch (typeName.nodeType) { + case 'ArrayTypeName': + return findUserDefinedType(typeName.baseType); + case 'Mapping': + // only mapping values can possibly refer to other array, mapping, or user-defined types + return findUserDefinedType(typeName.valueType); + case 'UserDefinedTypeName': + return typeName; + default: + return undefined; + } +} From 4f7e0494418b7f2ac4f992ceac5489a9b34a895f Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 12 Jun 2024 11:19:59 -0400 Subject: [PATCH 20/22] Update packages/core/src/validate/run.ts Co-authored-by: Francisco --- packages/core/src/validate/run.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 214361df2..f970e3d7c 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -619,7 +619,8 @@ function findUserDefinedType(typeName: TypeName): UserDefinedTypeName | undefine return findUserDefinedType(typeName.valueType); case 'UserDefinedTypeName': return typeName; - default: + case 'ElementaryTypeName': + case 'FunctionTypeName': return undefined; } } From 7aaec9962280bea24b9c514598783c5ebd8a5229 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 12 Jun 2024 11:20:48 -0400 Subject: [PATCH 21/22] Add unreachable assertion --- packages/core/src/validate/run.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index f970e3d7c..2565314da 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -21,6 +21,7 @@ import { getFullyQualifiedName } from '../utils/contract-name'; import { getAnnotationArgs as getSupportedAnnotationArgs, getDocumentation } from '../utils/annotations'; import { getStorageLocationAnnotation, isNamespaceSupported } from '../storage/namespace'; import { UpgradesError } from '../error'; +import { assertUnreachable } from '../utils/assert'; export type ValidationRunData = Record; @@ -622,5 +623,7 @@ function findUserDefinedType(typeName: TypeName): UserDefinedTypeName | undefine case 'ElementaryTypeName': case 'FunctionTypeName': return undefined; + default: + return assertUnreachable(typeName); } } From cb89e4c3d9a225ee46dd0c29c2fc5c966ae825b6 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 12 Jun 2024 11:40:28 -0400 Subject: [PATCH 22/22] Bump version --- packages/core/CHANGELOG.md | 2 +- packages/core/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 187e4bc58..50bd237a3 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 1.34.0 (2024-06-12) - Fix storage layout comparison for function types, disallow internal functions in storage. ([#1032](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1032)) diff --git a/packages/core/package.json b/packages/core/package.json index 8df81305f..c12e5b53c 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/upgrades-core", - "version": "1.33.1", + "version": "1.34.0", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core", "license": "MIT",