Avoid validation error when function parameter has internal function pointer

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleases
+
+- Fix unexpected validation error when function parameter has internal function pointer.
+
 ## 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))

packages/core/contracts/test/Validations.sol

@@ -219,6 +219,13 @@ contract StructInternalFunctionPointer {
     }
 }
 
+contract StructInternalFunctionPointerUsed {
+    struct S {
+        function(bool) internal foo;
+    }
+    S s;
+}
+
 contract StructImpliedInternalFunctionPointer {
     struct S {
         function(bool) foo;
@@ -276,3 +283,14 @@ contract InternalFunctionPointer {
 contract ImpliedInternalFunctionPointer {
     function(bool) foo;
 }
+
+contract FunctionWithInternalFunctionPointer {
+    uint208 x;
+
+    function doOp(
+        function(uint208, uint208) view returns (uint208) op,
+        uint208 y
+    ) internal view returns (uint208) {
+        return op(x, y);
+    }
+}

packages/core/src/validate.test.ts

@@ -8,6 +8,7 @@ import {
   assertUpgradeSafe,
   ValidationOptions,
   RunValidation,
+  ValidationErrors,
 } from './validate';
 import { solcInputOutputDecoder } from './src-decoder';
 
@@ -38,11 +39,21 @@ test.before(async t => {
   }
 });
 
-function testValid(name: string, kind: ValidationOptions['kind'], valid: boolean) {
-  testOverride(name, kind, {}, valid);
+function testValid(name: string, kind: ValidationOptions['kind'], valid: boolean, numExpectedErrors?: number) {
+  testOverride(name, kind, {}, valid, numExpectedErrors);
 }
 
-function testOverride(name: string, kind: ValidationOptions['kind'], opts: ValidationOptions, valid: boolean) {
+function testOverride(
+  name: string,
+  kind: ValidationOptions['kind'],
+  opts: ValidationOptions,
+  valid: boolean,
+  numExpectedErrors?: number,
+) {
+  if (numExpectedErrors !== undefined && numExpectedErrors > 0 && valid) {
+    throw new Error('Cannot expect errors for a valid contract');
+  }
+
   const optKeys = Object.keys(opts);
   const describeOpts = optKeys.length > 0 ? '(' + optKeys.join(', ') + ')' : '';
   const testName = [valid ? 'accepts' : 'rejects', kind, name, describeOpts].join(' ');
@@ -52,7 +63,10 @@ function testOverride(name: string, kind: ValidationOptions['kind'], opts: Valid
     if (valid) {
       t.notThrows(assertUpgSafe);
     } else {
-      t.throws(assertUpgSafe);
+      const error = t.throws(assertUpgSafe) as ValidationErrors;
+      if (numExpectedErrors !== undefined) {
+        t.is(error.errors.length, numExpectedErrors);
+      }
     }
   });
 }
@@ -142,6 +156,7 @@ testValid('contracts/test/ValidationsSameNameUnsafe.sol:SameName', 'transparent'
 
 testValid('StructExternalFunctionPointer', 'transparent', true);
 testValid('StructInternalFunctionPointer', 'transparent', false);
+testValid('StructInternalFunctionPointerUsed', 'transparent', false, 1);
 testValid('StructImpliedInternalFunctionPointer', 'transparent', false);
 testOverride(
   'StructImpliedInternalFunctionPointer',
@@ -163,6 +178,8 @@ testValid('InternalFunctionPointer', 'transparent', false);
 testValid('ImpliedInternalFunctionPointer', 'transparent', false);
 testOverride('ImpliedInternalFunctionPointer', 'transparent', { unsafeAllow: ['internal-function-storage'] }, true);
 
+testValid('FunctionWithInternalFunctionPointer', 'transparent', true);
+
 test('ambiguous name', t => {
   const error = t.throws(() => getContractVersion(t.context.validation, 'SameName'));
   t.is(

packages/core/src/validate/run.ts

@@ -6,6 +6,7 @@ import type {
   StructDefinition,
   TypeName,
   UserDefinedTypeName,
+  VariableDeclaration,
 } from 'solidity-ast';
 import debug from '../utils/debug';
 
@@ -586,7 +587,7 @@ function* getInternalFunctionStorageErrors(
 ): Generator<ValidationError> {
   // 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)) {
+  for (const variableDec of getVariableDeclarations(contractOrStructDef, visitedNodeIds)) {
     if (variableDec.typeName?.nodeType === 'FunctionTypeName' && variableDec.typeName.visibility === 'internal') {
       // Find internal function types directly in this node's scope
       yield {
@@ -608,6 +609,33 @@ function* getInternalFunctionStorageErrors(
   }
 }
 
+/**
+ * Gets variables declared directly in a contract and its struct definitions, or in a struct definition.
+ */
+function getVariableDeclarations(
+  contractOrStructDef: ContractDefinition | StructDefinition,
+  visitedNodeIds: Set<number>,
+): VariableDeclaration[] {
+  const results: VariableDeclaration[] = [];
+  if (contractOrStructDef.nodeType === 'ContractDefinition') {
+    for (const node of contractOrStructDef.nodes) {
+      if (node.nodeType === 'VariableDeclaration') {
+        results.push(node);
+      } else if (node.nodeType === 'StructDefinition' && !visitedNodeIds.has(node.id)) {
+        visitedNodeIds.add(node.id);
+        results.push(...getVariableDeclarations(node, visitedNodeIds));
+      }
+    }
+  } else if (contractOrStructDef.nodeType === 'StructDefinition') {
+    for (const member of contractOrStructDef.members) {
+      if (member.nodeType === 'VariableDeclaration') {
+        results.push(member);
+      }
+    }
+  }
+  return results;
+}
+
 /**
  * Recursively traverse array and mapping types to find user-defined types (which may be struct references).
  */