search

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

finding 46 add erc165 #118

Closed livingrockrises closed 3 months ago

livingrockrises commented 3 months ago

need to check which all interfaces we need to add to be compliant with the ERC7579. it's not there in reference implementation so I would probably open a PR there too and get feedback.

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/db118d211549108acdfeda08ab92fd67d4c3432e/contracts/contracts/base/RegistryAdapter.sol#L12)) should be constant [base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/db118d211549108acdfeda08ab92fd67d4c3432e/contracts/contracts/base/RegistryAdapter.sol#L12) - [ ] ID-1 [RegistryFactory.threshold]([factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/db118d211549108acdfeda08ab92fd67d4c3432e/contracts/contracts/factory/RegistryFactory.sol#L39)) should be constant [factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/db118d211549108acdfeda08ab92fd67d4c3432e/contracts/contracts/factory/RegistryFactory.sol#L39)

This comment was automatically generated by the GitHub Actions workflow.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## remediations/cantina-spearbit #118 +/- ## ================================================================ Coverage ? 70.64% ================================================================ Files ? 13 Lines ? 678 Branches ? 126 ================================================================ Hits ? 479 Misses ? 199 Partials ? 0 ``` | [Files](https://app.codecov.io/gh/bcnmy/nexus/pull/118?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) | Coverage Δ | | |---|---|---| | [contracts/Nexus.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/118?src=pr&el=tree&filepath=contracts%2FNexus.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL05leHVzLnNvbA==) | `59.44% <0.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/bcnmy/nexus/pull/118?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/bcnmy/nexus/pull/118?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Last update [ad6af35...da0740c](https://app.codecov.io/gh/bcnmy/nexus/pull/118?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy).
livingrockrises commented 3 months ago

tests are missing + would i make sense to add IERC4337Account and ERC7484 ?

yeah. we should / will make a list of missing tests

interfaces I need to confirm with Rhinestone.

why did you even suggest ERC7484? issue is specific to ERC7579 needs of supported interfaces. https://cantina.xyz/code/d1d4b139-9705-4367-9468-297b7078674e/findings/46

feel it's merely IERC7579Account , (maybe IERC54337Account)

Aboudjem commented 3 months ago

tests are missing + would i make sense to add IERC4337Account and ERC7484 ?

yeah. we should / will make a list of missing tests

interfaces I need to confirm with Rhinestone.

why did you even suggest ERC7484? issue is specific to ERC7579 needs of supported interfaces. https://cantina.xyz/code/d1d4b139-9705-4367-9468-297b7078674e/findings/46

feel it's merely IERC7579Account , (maybe IERC54337Account)

for the factories I meant

livingrockrises commented 3 months ago

tests are missing + would i make sense to add IERC4337Account and ERC7484 ?

yeah. we should / will make a list of missing tests interfaces I need to confirm with Rhinestone. why did you even suggest ERC7484? issue is specific to ERC7579 needs of supported interfaces. https://cantina.xyz/code/d1d4b139-9705-4367-9468-297b7078674e/findings/46 feel it's merely IERC7579Account , (maybe IERC54337Account)

for the factories I meant

no this is ERC165 for the account what interfaces must be supported.

livingrockrises commented 3 months ago

I am gonna potentially close this if we make do using fallback handler.

livingrockrises commented 3 months ago

closing this as we would do this via fallback handler as discussed with zeroknots. need votes @filmakarov @VGabriel45 @Aboudjem

livingrockrises commented 3 months ago

closing this as of now..