bcnmy / nexus

Nexus by Biconomy: ERC-7579 Modular Smart Account for Enhanced Account Abstraction
https://github.com/bcnmy/nexus/wiki
MIT License
27 stars 5 forks source link

🔒 M-01 - Prevent Replay Attacks by Enforcing Signature Malleability Check #119

Closed Aboudjem closed 3 months ago

Aboudjem commented 3 months ago

M-01. Potential Replay Attack Vulnerability in Signature Verification Logic

github-actions[bot] commented 3 months ago

:robot: Slither Analysis Report :mag_right:

Slither report

# Slither report **THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [constable-states](#constable-states) (2 results) (Optimization) ## constable-states Impact: Optimization :red_circle: Confidence: High - [ ] ID-0 [RegistryAdapter.registry]([base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/98c4a1c85a0ce5f5494109bdaae1137a76c0fdb6/contracts/contracts/base/RegistryAdapter.sol#L12)) should be constant [base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/98c4a1c85a0ce5f5494109bdaae1137a76c0fdb6/contracts/contracts/base/RegistryAdapter.sol#L12) - [ ] ID-1 [RegistryFactory.threshold]([factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/98c4a1c85a0ce5f5494109bdaae1137a76c0fdb6/contracts/contracts/factory/RegistryFactory.sol#L39)) should be constant [factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/98c4a1c85a0ce5f5494109bdaae1137a76c0fdb6/contracts/contracts/factory/RegistryFactory.sol#L39)

This comment was automatically generated by the GitHub Actions workflow.

livingrockrises commented 3 months ago

Hang on.. in any case we should probably use oz ECDSA library directly instead of making s checks in our code.

VGabriel45 commented 3 months ago

Hang on.. in any case we should probably use oz ECDSA library directly instead of making s checks in our code.

It uses the same logic as in the OZ ECDSA library, the signature with the lower 's' value is considered valid. If we only need this check I think using the OZ ECDSA library will just increase the code size even more. @livingrockrises @Aboudjem

Aboudjem commented 3 months ago

Hang on.. in any case we should probably use oz ECDSA library directly instead of making s checks in our code.

It uses the same logic as in the OZ ECDSA library, the signature with the lower 's' value is considered valid. If we only need this check I think using the OZ ECDSA library will just increase the code size even more. @livingrockrises @Aboudjem

I agree with the code size concerns, that's something that will need to fix after the remediations.

But Libraries works in a way that it will extend the code size only for the used functions, so it will not add the whole library codesize

livingrockrises commented 3 months ago

Yeah you aren't inheriting really so it's not a codesize thing.. anyway I confirmed with spearbit auditors that this is not an issue so I am going to cancel this PR