Fixing Audit Findings #67

zeroknots · 2024-05-28T00:45:22Z

No description provided.

fixed threshold finding using associated storage to store attesters to be 4337 compliant

zeroknots · 2024-05-28T00:45:49Z

src/DataTypes.sol

@@ -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;


NOTE: using associated storage to comply with 4337

zeroknots · 2024-05-28T00:46:19Z

src/core/TrustManager.sol

@@ -119,7 +119,17 @@ abstract contract TrustManager is IRegistry {
        // use this condition to save gas
        else if (threshold == 1) {
            AttestationRecord storage $attestation = $getAttestation({ module: module, attester: attester });
-            $attestation.enforceValid(moduleType);
+            if ($attestation.checkValid(moduleType)) return;


NOTE: added this routine, to handle the case that threshold == 1, but the first attester doesnt have an attestation

* ⚡️ Adding msg.sender separation for factory calls

zeroknots · 2024-05-28T00:49:34Z

src/core/ModuleManager.sol

+ * In order to separate msg.sender context from registry,
+ * interactions with external Factories are done with this Trampoline contract.
+ */
+contract FactoryTrampoline {


NOTE: added factory call trampoline, so calls made to factory dont come from msg.sender == registry

zeroknots · 2024-05-28T00:59:20Z

src/core/TrustManagerExternalAttesterList.sol

-            if ($getAttestation(module, attesters[i]).checkValid(ZERO_MODULE_TYPE)) {
+            address attester = attesters[i];
+            if (attester < _attesterCache) revert InvalidTrustedAttesterInput();
+            else _attesterCache = attester;


NOTE: adding check here to ensure provided list is sorted and unique. thus deduped

src/core/TrustManager.sol

wip

zeroknots · 2024-05-28T01:09:24Z

src/core/TrustManager.sol

-        // if attesters array has duplicates, revert
-        if (attestersLength == 0) revert InvalidTrustedAttesterInput();
+        // if attesters array has duplicates or is too large revert
+        if (attestersLength == 0 || attestersLength > type(uint8).max) revert InvalidTrustedAttesterInput();


NOTE: adding check to prevent attester length to be over uint8 thus preventing cast issue

zeroknots · 2024-05-28T01:15:19Z

src/core/ModuleManager.sol

@@ -48,7 +80,7 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
        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);


Note: making revert message consistent

zeroknots · 2024-05-28T01:15:23Z

src/core/ModuleManager.sol

@@ -73,7 +105,7 @@ abstract contract ModuleManager is IRegistry, ResolverManager {
        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);


Note: making revert message consistent

…ers are set

zeroknots · 2024-05-28T01:20:09Z

src/core/TrustManager.sol

@@ -149,12 +159,15 @@ abstract contract TrustManager is IRegistry {
        uint256 count = $trustedAttesters.attesterCount;
        address attester0 = $trustedAttesters.attester;

+        // return if no trusted attesters are set
+        if (count == 0) return attesters;


NOTE: prevent revert in view function if no attesters were set

…orted array, and not sort in in memory to safe gas

src/core/TrustManager.sol

zeroknots · 2024-05-28T01:31:01Z

src/core/TrustManager.sol

        if (attesters.length != attestersLength) revert InvalidTrustedAttesterInput();
+        // revert if the first attester is the zero address
+        // other attesters can not be zero address, as the array was sorted
+        if (attesters[0] == ZERO_ADDRESS) revert InvalidTrustedAttesterInput();


NOTE: checking address(0)

src/lib/TrustLib.sol

zeroknots · 2024-05-28T01:35:02Z

src/lib/TrustLib.sol

@@ -96,7 +96,7 @@ library TrustLib {
         */

        assembly {
-            let mask := 0xffffffffffff


NOTE: fixing bitmask

zeroknots · 2024-05-28T01:37:31Z

src/DataTypes.sol

@@ -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) {


NOTE: fixing inconsistent param namings

stacked PR audit: fix EIP712

openzeppelin-code · 2024-06-14T12:48:58Z

Fixing Audit Findings

Generated at commit: aaa9ce117b9cf1c9871ba4d940438f7a72d7a2ec

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 1 0 5 25 33
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

fix: inconsistencies

* fix: eip 712 according to ackee's feedback

wip

fix: attester storage

13a7bd9

fixed threshold finding using associated storage to store attesters to be 4337 compliant