Skip to content

Commit

Permalink
fix: attester storage
Browse files Browse the repository at this point in the history
fixed threshold finding

using associated storage to store attesters to be 4337 compliant
  • Loading branch information
zeroknots committed May 28, 2024
1 parent 2bb0d6b commit 13a7bd9
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 6 deletions.
4 changes: 4 additions & 0 deletions remappings.txt
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/
2 changes: 1 addition & 1 deletion src/DataTypes.sol
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
18 changes: 14 additions & 4 deletions src/core/TrustManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ abstract contract TrustManager is IRegistry {
address _attester = attesters[i];
// user could have set attester to address(0)
if (_attester == ZERO_ADDRESS) revert InvalidTrustedAttesterInput();
$trustedAttester.linkedAttesters[_attester] = attesters[i + 1];
$trustedAttester.linkedAttesters[_attester][msg.sender] = attesters[i + 1];
}

emit NewTrustedAttesters();
Expand Down Expand Up @@ -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;

// if first attestation is not valid, iterate over the linked list of attesters
// and check if the attestation is valid
for (uint256 i; i < attesterCount; i++) {
attester = $trustedAttesters.linkedAttesters[attester][smartAccount];
$attestation = $getAttestation({ module: module, attester: attester });
if ($attestation.checkValid(moduleType)) return;
}
// if no valid attestations were found in the for loop. the module is not valid
revert InsufficientAttestations();
}
// smart account has more than one trusted attester
else {
Expand All @@ -129,7 +139,7 @@ abstract contract TrustManager is IRegistry {

for (uint256 i = 1; i < attesterCount; i++) {
// get next attester from linked List
attester = $trustedAttesters.linkedAttesters[attester];
attester = $trustedAttesters.linkedAttesters[attester][smartAccount];
$attestation = $getAttestation({ module: module, attester: attester });
if ($attestation.checkValid(moduleType)) threshold--;

Expand All @@ -154,7 +164,7 @@ abstract contract TrustManager is IRegistry {

for (uint256 i = 1; i < count; i++) {
// get next attester from linked List
attesters[i] = $trustedAttesters.linkedAttesters[attesters[i - 1]];
attesters[i] = $trustedAttesters.linkedAttesters[attesters[i - 1]][smartAccount];
}
}

Expand Down
49 changes: 48 additions & 1 deletion test/TrustDelegation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ pragma solidity ^0.8.0;
import "./Attestation.t.sol";
import "src/DataTypes.sol";
import { LibSort } from "solady/utils/LibSort.sol";
import { VmSafe } from "forge-std/Vm.sol";
import { ERC4337SpecsParser } from "erc4337-validation/SpecsParser.sol";

contract TrustTest is AttestationTest {
using LibSort for address[];
using ERC4337SpecsParser for VmSafe.AccountAccess;

function setUp() public override {
super.setUp();
Expand All @@ -31,7 +34,7 @@ contract TrustTest is AttestationTest {
uint8 threshold,
address[] memory attesters
)
external
public
whenSettingAttester
prankWithAccount(smartAccount1)
{
Expand Down Expand Up @@ -123,4 +126,48 @@ contract TrustTest is AttestationTest {
vm.expectRevert();
registry.check(address(module1), ModuleType.wrap(3));
}

function test_WhenAttestersSetCheckOnlyOneThreshold() external whenQueryingRegistry {
test_WhenUsingValidECDSA();

vm.startPrank(smartAccount1.addr);
// It should not revert.
address[] memory attesters = new address[](2);
attesters[0] = address(makeAddr("foo"));
attesters[1] = address(attester1.addr);
registry.trustAttesters(1, attesters);

registry.check(address(module1), ModuleType.wrap(1));
registry.check(address(module1), ModuleType.wrap(2));
vm.expectRevert();
registry.check(address(module1), ModuleType.wrap(3));
}

function test_WhenSupplyingManyAttesters_ShouldBe4337Compliant(uint8 threshold, address[] memory attesters) public {
vm.startMappingRecording();
vm.startStateDiffRecording();

test_WhenSupplyingManyAttesters(threshold, attesters);

VmSafe.AccountAccess[] memory accesses = vm.stopAndReturnStateDiff();

ERC4337SpecsParser.Entities memory entities = ERC4337SpecsParser.Entities({
account: smartAccount1.addr,
factory: address(0),
isFactoryStaked: false,
paymaster: address(0),
isPaymasterStaked: false,
aggregator: address(0),
isAggregatorStaked: false
});

for (uint256 i; i < accesses.length; i++) {
VmSafe.AccountAccess memory currentAccess = accesses[i];
if (currentAccess.account != address(this) && currentAccess.accessor != address(this)) {
currentAccess.validateBannedStorageLocations(entities);
}
}

vm.stopMappingRecording();
}
}

0 comments on commit 13a7bd9

Please sign in to comment.