Merge pull request #67 from rhinestonewtf/audit/fix

Fixing Audit Findings

Author: kopy-kat

.solhint.json

@@ -2,7 +2,7 @@
   "extends": "solhint:recommended",
   "rules": {
     "avoid-low-level-calls": "off",
-    "code-complexity": ["error", 9],
+    "code-complexity": ["error", 11],
     "compiler-version": ["error", ">=0.8.0"],
     "contract-name-camelcase": "off",
     "const-name-snakecase": "off",

package.json

@@ -25,14 +25,14 @@
     "spellcheck": "cspell '**'"
   },
   "dependencies": {
-    "@openzeppelin/contracts": "5.0.1"
+    "@openzeppelin/contracts": "5.0.1",
+    "solady": "github:vectorized/solady#9deb9ed36a27261a8745db5b7cd7f4cdc3b1cd4e",
+    "forge-std": "github:foundry-rs/forge-std#v1.7.6"
   },
   "devDependencies": {
     "@defi-wonderland/natspec-smells": "^1.1.1",
     "cspell": "^8.6.0",
     "ds-test": "github:dapphub/ds-test#e282159d5170298eb2455a6c05280ab5a73a4ef0",
-    "forge-std": "github:foundry-rs/forge-std#v1.7.6",
-    "solady": "github:vectorized/solady#9deb9ed36a27261a8745db5b7cd7f4cdc3b1cd4e",
     "solhint": "^4.5.2",
     "solmate": "github:transmissions11/solmate#c892309933b25c03d32b1b0d674df7ae292ba925",
     "@rhinestone/erc4337-validation": "github:rhinestonewtf/erc4337-validation"

pnpm-lock.yaml

@@ -3,3 +3,7 @@ solmate/=node_modules/solmate/src/
 solady/=node_modules/solady/src/
 forge-std/=node_modules/forge-std/src/
 ds-test/=node_modules/ds-test/src/
+erc4337-validation/=node_modules/@rhinestone/erc4337-validation/src/
+account-abstraction/=node_modules/account-abstraction/contracts/
+account-abstraction-v0.6/=node_modules/account-abstraction-v0.6/contracts/
+@openzeppelin/=node_modules/@openzeppelin/

remappings.txt

@@ -41,7 +41,7 @@ struct TrustedAttesterRecord {
     uint8 attesterCount; // number of attesters in the linked list
     uint8 threshold; // minimum number of attesters required
     address attester; // first attester in linked list. (packed to save gas)
-    mapping(address attester => address linkedAttester) linkedAttesters; // linked list
+    mapping(address attester => mapping(address account => address linkedAttester)) linkedAttesters;
 }
 
 /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
@@ -75,12 +75,12 @@ type SchemaUID is bytes32;
 using { schemaEq as == } for SchemaUID global;
 using { schemaNotEq as != } for SchemaUID global;
 
-function schemaEq(SchemaUID uid1, SchemaUID uid) pure returns (bool) {
-    return SchemaUID.unwrap(uid1) == SchemaUID.unwrap(uid);
+function schemaEq(SchemaUID uid1, SchemaUID uid2) pure returns (bool) {
+    return SchemaUID.unwrap(uid1) == SchemaUID.unwrap(uid2);
 }
 
-function schemaNotEq(SchemaUID uid1, SchemaUID uid) pure returns (bool) {
-    return SchemaUID.unwrap(uid1) != SchemaUID.unwrap(uid);
+function schemaNotEq(SchemaUID uid1, SchemaUID uid2) pure returns (bool) {
+    return SchemaUID.unwrap(uid1) != SchemaUID.unwrap(uid2);
 }
 
 //--------------------- ResolverUID -----------------------------|

src/DataTypes.sol

@@ -13,43 +13,9 @@ import {
     SchemaUID,
     SchemaRecord
 } from "./DataTypes.sol";
-
 import { IExternalSchemaValidator } from "./external/IExternalSchemaValidator.sol";
 import { IExternalResolver } from "./external/IExternalResolver.sol";
-
-interface IERC7484 {
-    event NewTrustedAttesters();
-    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
-    /*          Check with Registry internal attesters            */
-    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
-
-    function check(address module) external view;
-
-    function checkForAccount(address smartAccount, address module) external view;
-
-    function check(address module, ModuleType moduleType) external view;
-
-    function checkForAccount(address smartAccount, address module, ModuleType moduleType) external view;
-
-    /**
-     * Allows Smart Accounts - the end users of the registry - to appoint
-     * one or many attesters as trusted.
-     * @dev this function reverts, if address(0), or duplicates are provided in attesters[]
-     *
-     * @param threshold The minimum number of attestations required for a module
-     *                  to be considered secure.
-     * @param attesters The addresses of the attesters to be trusted.
-     */
-    function trustAttesters(uint8 threshold, address[] calldata attesters) external;
-
-    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
-    /*              Check with external attester(s)               */
-    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
-
-    function check(address module, address[] calldata attesters, uint256 threshold) external view;
-
-    function check(address module, ModuleType moduleType, address[] calldata attesters, uint256 threshold) external view;
-}
+import { IERC7484 } from "./interfaces/IERC7484.sol";
 
 /**
  * Interface definition of all features of the registry:
@@ -91,6 +57,7 @@ interface IRegistry is IERC7484 {
     event Attested(address indexed moduleAddr, address indexed attester, SchemaUID schemaUID, AttestationDataRef indexed sstore2Pointer);
 
     error AlreadyRevoked();
+    error AlreadyAttested();
     error ModuleNotFoundInRegistry(address module);
     error AccessDenied();
     error InvalidAttestation();
@@ -236,12 +203,14 @@ interface IRegistry is IERC7484 {
      * @param metadata The metadata to be stored on the registry.
      *            This field is optional, and might be used by the module developer to store additional
      *            information about the module or facilitate business logic with the Resolver stub
+     * @param resolverContext bytes that will be passed to the resolver contract
      */
     function deployModule(
         bytes32 salt,
         ResolverUID resolverUID,
         bytes calldata initCode,
-        bytes calldata metadata
+        bytes calldata metadata,
+        bytes calldata resolverContext
     )
         external
         payable
@@ -250,7 +219,9 @@ interface IRegistry is IERC7484 {
     /**
      * In order to make the integration into existing business logics possible,
      * the Registry is able to utilize external factories that can be utilized to deploy the modules.
-     * @dev Registry can use other factories to deploy the module
+     * @dev Registry can use other factories to deploy the module.
+     * @dev Note that this function will call the external factory via the FactoryTrampoline contract.
+     *           Factory MUST not assume that msg.sender == registry
      * @dev This function is used to deploy and register a module using a factory contract.
      *           Since one of the parameters of this function is a unique resolverUID and any
      *           registered module address can only be registered once,
@@ -260,7 +231,8 @@ interface IRegistry is IERC7484 {
         address factory,
         bytes calldata callOnFactory,
         bytes calldata metadata,
-        ResolverUID resolverUID
+        ResolverUID resolverUID,
+        bytes calldata resolverContext
     )
         external
         payable
@@ -278,8 +250,15 @@ interface IRegistry is IERC7484 {
      * @param metadata The metadata to be stored on the registry.
      *            This field is optional, and might be used by the module developer to store additional
      *            information about the module or facilitate business logic with the Resolver stub
+     * @param resolverContext bytes that will be passed to the resolver contract
      */
-    function registerModule(ResolverUID resolverUID, address moduleAddress, bytes calldata metadata) external;
+    function registerModule(
+        ResolverUID resolverUID,
+        address moduleAddress,
+        bytes calldata metadata,
+        bytes calldata resolverContext
+    )
+        external;
 
     /**
      * in conjunction with the deployModule() function, this function let's you

src/IRegistry.sol

@@ -122,6 +122,11 @@ abstract contract AttestationManager is IRegistry, ModuleManager, SchemaManager,
         }
         // caching module address.
         address module = request.moduleAddr;
+        AttestationRecord storage $record = $getAttestation({ module: module, attester: attester });
+        // If the attestation was already made for module, but not revoked, revert.
+        if ($record.time != ZERO_TIMESTAMP && $record.revocationTime == ZERO_TIMESTAMP) {
+            revert AlreadyAttested();
+        }
         // SLOAD the resolverUID from the moduleRecord
         resolverUID = $moduleAddrToRecords[module].resolverUID;
         // Ensure that attestation is for module that was registered.

src/core/AttestationManager.sol

@@ -9,6 +9,32 @@ import { ResolverRecord, ModuleRecord, ResolverUID } from "../DataTypes.sol";
 import { ResolverManager } from "./ResolverManager.sol";
 import { IRegistry } from "../IRegistry.sol";
 
+/**
+ * In order to separate msg.sender context from registry,
+ * interactions with external Factories are done with this Trampoline contract.
+ */
+contract FactoryTrampoline {
+    error FactoryCallFailed(address factory);
+
+    /**
+     * @param factory the address of the factory to call
+     * @param callOnFactory the call data to send to the factory
+     * @return moduleAddress the moduleAddress that was returned by the
+     */
+    function deployViaFactory(address factory, bytes memory callOnFactory) external payable returns (address moduleAddress) {
+        // call external factory to deploy module
+        bool success;
+        /* solhint-disable no-inline-assembly */
+        assembly ("memory-safe") {
+            success := call(gas(), factory, callvalue(), add(callOnFactory, 0x20), mload(callOnFactory), 0, 32)
+            moduleAddress := mload(0)
+        }
+        if (!success) {
+            revert FactoryCallFailed(factory);
+        }
+    }
+}
+
 /**
  * In order for Attesters to be able to make statements about a Module, the Module first needs to be registered on the Registry.
  * This can be done as part of or after Module deployment. On registration, every module is tied to a
@@ -34,29 +60,40 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
 
     mapping(address moduleAddress => ModuleRecord moduleRecord) internal $moduleAddrToRecords;
 
+    FactoryTrampoline private immutable FACTORY_TRAMPOLINE;
+
+    constructor() {
+        FACTORY_TRAMPOLINE = new FactoryTrampoline();
+    }
+
     /**
      * @inheritdoc IRegistry
      */
     function deployModule(
         bytes32 salt,
         ResolverUID resolverUID,
         bytes calldata initCode,
-        bytes calldata metadata
+        bytes calldata metadata,
+        bytes calldata resolverContext
     )
         external
         payable
         returns (address moduleAddress)
     {
         ResolverRecord storage $resolver = $resolvers[resolverUID];
-        if ($resolver.resolverOwner == ZERO_ADDRESS) revert InvalidResolver($resolver.resolver);
+        if ($resolver.resolverOwner == ZERO_ADDRESS) revert InvalidResolverUID(resolverUID);
 
         moduleAddress = initCode.deploy(salt);
         // _storeModuleRecord() will check if module is already registered,
         // which should prevent reentry to any deploy function
         ModuleRecord memory record =
             _storeModuleRecord({ moduleAddress: moduleAddress, sender: msg.sender, resolverUID: resolverUID, metadata: metadata });
 
-        record.requireExternalResolverOnModuleRegistration({ moduleAddress: moduleAddress, $resolver: $resolver });
+        record.requireExternalResolverOnModuleRegistration({
+            moduleAddress: moduleAddress,
+            $resolver: $resolver,
+            resolverContext: resolverContext
+        });
     }
 
     /**
@@ -69,11 +106,18 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
     /**
      * @inheritdoc IRegistry
      */
-    function registerModule(ResolverUID resolverUID, address moduleAddress, bytes calldata metadata) external {
+    function registerModule(
+        ResolverUID resolverUID,
+        address moduleAddress,
+        bytes calldata metadata,
+        bytes calldata resolverContext
+    )
+        external
+    {
         ResolverRecord storage $resolver = $resolvers[resolverUID];
 
         // ensure that non-zero resolverUID was provided
-        if ($resolver.resolverOwner == ZERO_ADDRESS) revert InvalidResolver($resolver.resolver);
+        if ($resolver.resolverOwner == ZERO_ADDRESS) revert InvalidResolverUID(resolverUID);
 
         ModuleRecord memory record = _storeModuleRecord({
             moduleAddress: moduleAddress,
@@ -83,7 +127,11 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
         });
 
         // resolve module registration
-        record.requireExternalResolverOnModuleRegistration({ moduleAddress: moduleAddress, $resolver: $resolver });
+        record.requireExternalResolverOnModuleRegistration({
+            moduleAddress: moduleAddress,
+            $resolver: $resolver,
+            resolverContext: resolverContext
+        });
     }
 
     /**
@@ -93,7 +141,8 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
         address factory,
         bytes calldata callOnFactory,
         bytes calldata metadata,
-        ResolverUID resolverUID
+        ResolverUID resolverUID,
+        bytes calldata resolverContext
     )
         external
         payable
@@ -105,11 +154,9 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
         // prevent someone from calling a registry function pretending its a factory
         if (factory == address(this)) revert FactoryCallFailed(factory);
 
-        // call external factory to deploy module
-        (bool ok, bytes memory returnData) = factory.call{ value: msg.value }(callOnFactory);
-        if (!ok) revert FactoryCallFailed(factory);
-
-        moduleAddress = abi.decode(returnData, (address));
+        // Call the factory via the trampoline contract. This will make sure that there is msg.sender separation
+        // Making "raw" calls to user supplied addresses could create security issues.
+        moduleAddress = FACTORY_TRAMPOLINE.deployViaFactory{ value: msg.value }({ factory: factory, callOnFactory: callOnFactory });
 
         ModuleRecord memory record = _storeModuleRecord({
             moduleAddress: moduleAddress,
@@ -118,7 +165,11 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
             metadata: metadata
         });
 
-        record.requireExternalResolverOnModuleRegistration({ moduleAddress: moduleAddress, $resolver: $resolver });
+        record.requireExternalResolverOnModuleRegistration({
+            moduleAddress: moduleAddress,
+            $resolver: $resolver,
+            resolverContext: resolverContext
+        });
     }
 
     /**

src/core/ModuleManager.sol

@@ -23,6 +23,16 @@ abstract contract SchemaManager is IRegistry {
     // The global mapping between schema records and their IDs.
     mapping(SchemaUID uid => SchemaRecord schemaRecord) internal schemas;
 
+    /**
+     * If a validator is not address(0), we check if it supports the IExternalSchemaValidator interface
+     */
+    modifier onlySchemaValidator(IExternalSchemaValidator validator) {
+        if (address(validator) != address(0) && !validator.supportsInterface(type(IExternalSchemaValidator).interfaceId)) {
+            revert InvalidSchemaValidator(validator);
+        }
+        _;
+    }
+
     /**
      * @inheritdoc IRegistry
      */
@@ -49,16 +59,6 @@ abstract contract SchemaManager is IRegistry {
         emit SchemaRegistered(uid, msg.sender);
     }
 
-    /**
-     * If a validator is not address(0), we check if it supports the IExternalSchemaValidator interface
-     */
-    modifier onlySchemaValidator(IExternalSchemaValidator validator) {
-        if (address(validator) != address(0) && !validator.supportsInterface(type(IExternalSchemaValidator).interfaceId)) {
-            revert InvalidSchemaValidator(validator);
-        }
-        _;
-    }
-
     /**
      * @inheritdoc IRegistry
      */