Skip to content

Commit

Permalink
Merge pull request #67 from rhinestonewtf/audit/fix
Browse files Browse the repository at this point in the history 
Fixing Audit Findings
  • Loading branch information
@kopy-kat
kopy-kat authored Jul 3, 2024
2 parents fe0735f + aaa9ce1 commit 4c29657
Show file tree
Hide file tree
Showing 25 changed files with 891 additions and 720 deletions.
2 changes: 1 addition & 1 deletion .solhint.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1,056 changes: 477 additions & 579 deletions pnpm-lock.yaml
View file

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions remappings.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/
10 changes: 5 additions & 5 deletions src/DataTypes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
Expand Down Expand Up @@ -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 -----------------------------|
Expand Down
57 changes: 18 additions & 39 deletions src/IRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand Down
5 changes: 5 additions & 0 deletions src/core/AttestationManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
77 changes: 64 additions & 13 deletions src/core/ModuleManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
});
}

/**
Expand All @@ -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,
Expand All @@ -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
});
}

/**
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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
});
}

/**
Expand Down
20 changes: 10 additions & 10 deletions src/core/SchemaManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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
*/
Expand Down
Loading

0 comments on commit 4c29657

Please sign in to comment.