Fix/finding #2 Fix Calculating Salt #124

Aboudjem · 2024-08-01T14:35:43Z

This PR addresses security audit finding #2 regarding non-deterministic salt computation in the createAccount function across several factory contracts. The following updates were made:

K1ValidatorFactory:
- Updated NatSpec documentation.
- Fixed non-deterministic salt issue by ensuring all parameters are correctly hashed together.
NexusAccountFactory:
- Rewritten assembly code for correct salt computation.
- Added missing NatSpec documentation.
RegistryFactory:
- Corrected assembly code for deterministic salt computation.
- Optimized gas usage by changing isModuleAllowed from public to private.
Test Cases:
- Added tests to verify that manually computed addresses match those generated by the updated computeAccountAddress function, ensuring the correctness of the salt computation fix.

…reshold parameters in createAccount and computeAccountAddress functions

… threshold parameters in createAccount and computeAccountAddress functions

…ture and improve readability

openzeppelin-code · 2024-08-01T14:38:56Z

Fix/finding #2 Fix Calculating Salt

Generated at commit: 9024384c238c7ce92105726fc62988fd885875d3

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 1 0 6 24 31

For more details view the full report in OpenZeppelin Code Inspector

codecov · 2024-08-01T14:45:30Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (remediations/cantina-spearbit@ad6af35). Learn more about missing BASE report.

Files	Patch %	Lines
contracts/factory/RegistryFactory.sol	75.00%	2 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##             remediations/cantina-spearbit     #124   +/-   ##
================================================================
  Coverage                                 ?   75.73%           
================================================================
  Files                                    ?       13           
  Lines                                    ?      643           
  Branches                                 ?      148           
================================================================
  Hits                                     ?      487           
  Misses                                   ?      156           
  Partials                                 ?        0

Files	Coverage Δ
contracts/factory/K1ValidatorFactory.sol	`100.00% <100.00%> (ø)`
contracts/factory/NexusAccountFactory.sol	`100.00% <100.00%> (ø)`
contracts/factory/RegistryFactory.sol	`80.48% <75.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad6af35...9024384. Read the comment docs.

livingrockrises · 2024-08-10T16:06:08Z

contracts/factory/K1ValidatorFactory.sol

- mstore(0x40, add(ptr, calldataLength))
- calldatacopy(ptr, 0x04, calldataLength)
- actualSalt := keccak256(ptr, calldataLength)
- }


not quite sure why these two are not the same?

I believe this change was done only for gas optimisation from gaslite suggestion.

you're effectively calculating keccak256(abi.encode(...params)) while his code is keccak256(abi.encodePacked(...params). Generally encodePack-ing is cheaper but I can't say for sure in this case if re-encoding will be cheaper than copying and hashing pre-encoded data. Best to look at benchmarks.

check how arrays are abi encoded, there's an offset to the data (that can be chosen by the caller as the issue says) and you'd be encoding this offset too along with everything else.

livingrockrises · 2024-08-10T16:09:03Z

audit finding #2 is linked wrong. it links to some issue

livingrockrises · 2024-08-10T16:16:18Z

contracts/factory/NexusAccountFactory.sol

- calldatacopy(ptr, 0x04, calldataLength)
- actualSalt := keccak256(ptr, calldataLength)
+ // Store initData at the free memory pointer
+ calldatacopy(ptr, initData.offset, initData.length)


why not consistent everywhere?
here it would be
bytes32 actualSalt = keccak256(abi.encodePacked(initData, salt))

that's what it does but in assembly

I mean, why cant we just use this abi.encodePacked way everywhere then.

It's for gas mostly.

But where else it's not used?

like in other factories we have changed and used like this
bytes32 actualSalt = keccak256(abi.encodePacked(eoaOwner, index, attesters, threshold));

go back to the issue please to also understand test case needs.
we are still hashing everything for salt event without any change, but in the event it would be emitted different value which won't help to reconstruct salt

@Aboudjem
cc @MrToph

livingrockrises · 2024-08-10T16:27:52Z

contracts/factory/NexusAccountFactory.sol

@@ -42,14 +42,17 @@ contract NexusAccountFactory is Stakeable, INexusFactory {
 /// @param salt Unique salt for the Smart Account creation.
 /// @return The address of the newly created Nexus account.
 function createAccount(bytes calldata initData, bytes32 salt) external payable override returns (address payable) {
- // Compute the actual salt for deterministic deployment
+ // Compute the actual salt as the hash of initData and salt using assembly


whats wrong with this comment

// Compute the actual salt for deterministic deployment

I changed it to reflect the changes, as we use hash of initData and Salt only (without sig func)

for deterministic deployment

Is nice and consistent..

livingrockrises · 2024-08-10T16:29:14Z

test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree

@@ -11,5 +11,7 @@ TestK1ValidatorFactory_Deployments
 │ └── it should return the expected address
 ├── when creating an account with the same owner and index
 │ └── it should result in the same address
- └── when creating accounts with different indexes
- └── it should result in different addresses
+ ├── when creating accounts with different indexes


I think tests should confirm that you're able to compute same account address using the initData and salt index emitted in the events.

or could be different params depending on the factory/method but you get the gist.

The written tests are valid in my opinion

livingrockrises

review i

VGabriel45

@Aboudjem look over Chirag's comments

…ePacked

Aboudjem · 2024-08-12T15:22:06Z

please check

github-actions · 2024-08-12T15:22:55Z

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

constable-states (2 results) (Optimization)

constable-states

Impact: Optimization
🔴 Confidence: High

ID-0
RegistryAdapter.registry should be constant

base/RegistryAdapter.sol#L12

ID-1
RegistryFactory.threshold should be constant

factory/RegistryFactory.sol#L39

_This comment was automatically generated by the GitHub Actions workflow._

Aboudjem added 8 commits August 1, 2024 16:30

chore: Update K1ValidatorFactory contract to include attesters and th…

50c8b93

…reshold parameters in createAccount and computeAccountAddress functions

refactor NexusAccountFactory to compute actual salt using assembly

17ab61b

refactor RegistryFactory to use private _isModuleAllowed function

507810b

feat: Add test for manually computed account address comparison

6015bfb

refactor: Update TestAccountFactory_Deployments.tree test structure

364bb0d

refactor: Update K1ValidatorFactory contract to include attesters and…

cf86649

… threshold parameters in createAccount and computeAccountAddress functions

refactor: Update TestK1ValidatorFactory_Deployments.tree test structure

c48ca23

refactor: Update TestNexusAccountFactory_Deployments.t.sol test struc…

7b8bdb9

…ture and improve readability

Aboudjem requested review from filmakarov, ankurdubey521, VGabriel45 and livingrockrises August 1, 2024 14:35

Aboudjem self-assigned this Aug 1, 2024

livingrockrises reviewed Aug 10, 2024

View reviewed changes

livingrockrises changed the title ~~Fix/finding #2 Fix Salt Determinism~~ Fix/finding #2 Fix Calculating Salt Aug 10, 2024

livingrockrises reviewed Aug 10, 2024

View reviewed changes

VGabriel45 reviewed Aug 12, 2024

View reviewed changes

Aboudjem added 2 commits August 12, 2024 17:20

refactor: Compute actual salt for NexusAccountFactory using abi.encod…

4479307

…ePacked

refactor: Compute actual salt for NexusAccountFactory using abi.encod…

9024384

…ePacked

Aboudjem requested review from VGabriel45 and livingrockrises August 12, 2024 15:22

MrToph approved these changes Aug 15, 2024

View reviewed changes

livingrockrises approved these changes Aug 16, 2024

View reviewed changes

livingrockrises merged commit 0070740 into remediations/cantina-spearbit Aug 16, 2024
9 of 10 checks passed

livingrockrises deleted the fix/finding-2-salt-determinism branch August 16, 2024 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/finding #2 Fix Calculating Salt #124

Fix/finding #2 Fix Calculating Salt #124

Aboudjem commented Aug 1, 2024

openzeppelin-code bot commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

livingrockrises Aug 10, 2024

ankurdubey521 Aug 12, 2024 •

edited

Loading

MrToph Aug 13, 2024

livingrockrises commented Aug 10, 2024

livingrockrises Aug 10, 2024

Aboudjem Aug 12, 2024

livingrockrises Aug 12, 2024

Aboudjem Aug 12, 2024

livingrockrises Aug 12, 2024

livingrockrises Aug 14, 2024

livingrockrises Aug 10, 2024

Aboudjem Aug 12, 2024

livingrockrises Aug 12, 2024

livingrockrises Aug 14, 2024

livingrockrises Aug 10, 2024

VGabriel45 Aug 12, 2024

livingrockrises left a comment

VGabriel45 left a comment

Aboudjem commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

constable-states

Fix/finding #2 Fix Calculating Salt #124

Fix/finding #2 Fix Calculating Salt #124

Conversation

Aboudjem commented Aug 1, 2024

openzeppelin-code bot commented Aug 1, 2024 • edited Loading

Fix/finding #2 Fix Calculating Salt

🚨 Report Summary

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ankurdubey521 Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises commented Aug 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises left a comment

Choose a reason for hiding this comment

VGabriel45 left a comment

Choose a reason for hiding this comment

Aboudjem commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

🤖 Slither Analysis Report 🔎

Slither report

constable-states

openzeppelin-code bot commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

ankurdubey521 Aug 12, 2024 •

edited

Loading