Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ERC7739 compatibility #241

Merged
merged 8 commits into from
Oct 16, 2024
Merged

feat: add ERC7739 compatibility #241

merged 8 commits into from
Oct 16, 2024

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented Oct 12, 2024

Switch the current ReplaySafeHash scheme to use an ERC7739 compatible signature scheme instead

Implementation uses pieces from, and/or is inspired by Solady and ERC7579's ERC7739 validator implementation

Copy link

octane-security-app bot commented Oct 12, 2024

Summary by Octane

New Contracts

  • ERC7739ReplaySafeWrapperLib.sol: The smart contract provides ERC-7739 nested EIP-712 wrappers for efficient, secure, and replay-safe ERC-1271 signature validation for smart contracts.

Updated Contracts

  • SemiModularAccountBase.sol: The smart contract modification removes duplicate import statements, obsolete replay-safe hash logic, and introduces ERC7739 signature format validation.
  • SemiModularKnownSelectorsLib.sol: Removed the replaySafeHash selector functionality from the smart contract.
  • SingleSignerValidationModule.sol: The smart contract replaces ReplaySafeWrapper with ERC7739ReplaySafeWrapperLib for signature validation and functionality enhancements.
  • WebAuthnValidationModule.sol: The smart contract's primary functionality now includes integration with a different BaseModule path to enhance module compatibility.

🔗 Commit Hash: a205db3

Copy link

github-actions bot commented Oct 12, 2024

Contract sizes:

 | Contract                      | Size (B) | Margin (B) |
 |-------------------------------|----------|------------|
 | AccountFactory                |    4,814 |     19,762 |
 | AllowlistModule               |    9,230 |     15,346 |
 | ModularAccount                |   25,167 |       -591 |
 | NativeTokenLimitModule        |    4,714 |     19,862 |
 | PaymasterGuardModule          |    1,797 |     22,779 |
-| SemiModularAccountBytecode    |   27,124 |     -2,548 |
-| SemiModularAccountStorageOnly |   27,642 |     -3,066 |
-| SingleSignerValidationModule  |    3,646 |     20,930 |
+| SemiModularAccountBytecode    |   27,775 |     -3,199 |
+| SemiModularAccountStorageOnly |   28,293 |     -3,717 |
+| SingleSignerValidationModule  |    4,300 |     20,276 |
 | TimeRangeModule               |    2,000 |     22,576 |
 | WebAuthnValidationModule      |    7,854 |     16,722 |

Code coverage:

File % Lines % Statements % Branches % Funcs
src/account/AccountStorageInitializable.sol 100.00% (19/19) 100.00% (26/26) 100.00% (5/5) 100.00% (2/2)
src/account/BaseAccount.sol 100.00% (8/8) 100.00% (7/7) 100.00% (2/2) 100.00% (4/4)
src/account/ModularAccount.sol 100.00% (1/1) 100.00% (1/1) 100.00% (0/0) 100.00% (2/2)
src/account/ModularAccountBase.sol 95.39% (290/304) 93.51% (360/385) 67.80% (40/59) 100.00% (38/38)
src/account/ModularAccountView.sol 100.00% (22/22) 100.00% (25/25) 100.00% (2/2) 100.00% (2/2)
src/account/ModuleManagerInternals.sol 88.37% (114/129) 87.06% (148/170) 52.94% (9/17) 100.00% (12/12)
src/account/SemiModularAccountBase.sol 86.89% (53/61) 90.70% (78/86) 66.67% (10/15) 94.12% (16/17)
src/account/SemiModularAccountBytecode.sol 100.00% (5/5) 100.00% (6/6) 100.00% (1/1) 50.00% (1/2)
src/account/SemiModularAccountStorageOnly.sol 100.00% (4/4) 100.00% (5/5) 100.00% (0/0) 100.00% (2/2)
src/account/TokenReceiver.sol 33.33% (1/3) 33.33% (1/3) 100.00% (0/0) 33.33% (1/3)
src/factory/AccountFactory.sol 70.59% (24/34) 76.09% (35/46) 40.00% (2/5) 58.33% (7/12)
src/libraries/ERC7739ReplaySafeWrapperLib.sol 85.88% (73/85) 85.87% (79/92) 66.67% (2/3) 87.50% (7/8)
src/libraries/ExecutionLib.sol 98.39% (244/248) 97.18% (241/248) 88.00% (22/25) 100.00% (22/22)
src/libraries/KnownSelectorsLib.sol 100.00% (29/29) 100.00% (64/64) 100.00% (0/0) 100.00% (3/3)
src/libraries/LinkedListSetLib.sol 94.00% (47/50) 96.25% (77/80) 66.67% (4/6) 100.00% (8/8)
src/libraries/MemManagementLib.sol 100.00% (54/54) 100.00% (70/70) 100.00% (0/0) 100.00% (12/12)
src/libraries/SemiModularKnownSelectorsLib.sol 100.00% (5/5) 100.00% (10/10) 100.00% (0/0) 100.00% (1/1)
src/modules/BaseModule.sol 100.00% (13/13) 94.12% (16/17) 100.00% (2/2) 100.00% (3/3)
src/modules/permissions/AllowlistModule.sol 86.05% (74/86) 85.71% (96/112) 78.26% (18/23) 50.00% (9/18)
src/modules/permissions/NativeTokenLimitModule.sol 87.80% (36/41) 89.66% (52/58) 88.89% (8/9) 54.55% (6/11)
src/modules/permissions/PaymasterGuardModule.sol 90.00% (9/10) 86.67% (13/15) 100.00% (2/2) 71.43% (5/7)
src/modules/permissions/TimeRangeModule.sol 83.33% (10/12) 80.00% (16/20) 100.00% (1/1) 75.00% (6/8)
src/modules/validation/SingleSignerValidationModule.sol 92.00% (23/25) 81.08% (30/37) 62.50% (5/8) 90.00% (9/10)
src/modules/validation/WebAuthnValidationModule.sol 61.11% (11/18) 66.67% (18/27) 100.00% (3/3) 60.00% (6/10)
Total 92.34% (1169/1266) 91.55% (1474/1610) 73.40% (138/188) 84.79% (184/217)

Copy link

octane-security-app bot commented Oct 12, 2024

Overview

Octane AI analysis has finished. No vulnerabilities were found. Cheers! 🎉🎉🎉


🔗 Commit Hash: a205db3

src/modules/validation/WebauthnValidationModule.sol Outdated Show resolved Hide resolved
test/modules/WebauthnValidationModule.t.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
@howydev howydev force-pushed the howy/7739-compatibility branch from 5d62bcf to aa4f20f Compare October 15, 2024 21:01
@@ -47,6 +47,7 @@ contract ModularAccountTest is AccountTestBase {
event ReceivedCall(bytes msgData, uint256 msgValue);

function setUp() public override {
_revertSnapshot = vm.snapshot();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more context on why this is here - in the repo, to do tests on MA + SMA we take a snapshot in the test constructor in AccountTestBase and this saves a copy of the chain state. The withSMATest modifier causes tests to run once on MAs, then does a revert to the snapshotted state and runs tests on SMAs

However, because it happens during the constructor, when the state is resetted, the test contract ends up having 0 bytecode. For some reason this causes external calls to revert which we rely on here as an easy way to convert bytes memory into bytes calldata

This PR moves the snapshot into setUp(), so when reverting to the snapshot, the test contract address will contain the bytecode with the required external helper functions

Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but we should investigate the fields left shift issue.

test/utils/ModuleSignatureUtils.sol Outdated Show resolved Hide resolved
test/utils/ModuleSignatureUtils.sol Outdated Show resolved Hide resolved
test/account/PerHookData.t.sol Show resolved Hide resolved
test/utils/ModuleSignatureUtils.sol Outdated Show resolved Hide resolved
src/account/SemiModularAccountBase.sol Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
src/helpers/ERC7739ReplaySafeWrapper.sol Outdated Show resolved Hide resolved
@howydev howydev force-pushed the howy/7739-compatibility branch 2 times, most recently from 7a697f3 to 7e14d10 Compare October 16, 2024 19:38
howydev and others added 2 commits October 16, 2024 15:39
chore: improve comments and dedupe constants

Co-authored-by: Adam Egyed <[email protected]>

Update src/helpers/ERC7739ReplaySafeWrapper.sol

Update src/helpers/ERC7739ReplaySafeWrapper.sol

Co-authored-by: Zer0dot <[email protected]>

Update src/helpers/ERC7739ReplaySafeWrapper.sol

Co-authored-by: Zer0dot <[email protected]>

Co-authored-by: Adam Egyed <[email protected]>

chore: gas bad

chore: fix test name

Co-authored-by: Adam Egyed <[email protected]>
@howydev howydev force-pushed the howy/7739-compatibility branch from 7e14d10 to 1148b11 Compare October 16, 2024 20:12
signingKey,
bytes memory deferredValidationSig;
bytes memory deferredValidationDatas;
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stack too deep

@howydev howydev merged commit 75a9622 into develop Oct 16, 2024
6 checks passed
@howydev howydev deleted the howy/7739-compatibility branch October 16, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants