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

#28 - Ensure _execute and _tryExecute Functions Correctly Handle Calls to EOAs #128

Closed Aboudjem closed 3 months ago

Aboudjem commented 3 months ago

Updated _execute and _tryExecute functions to include an extcodesize check, ensuring calls to EOAs are not incorrectly marked as successful. This change aligns the behavior of low-level calls with Solidity's expectations

github-actions[bot] commented 3 months ago

Changes to gas cost

Generated at commit: c654f5061ba8ac118b20169a31f048db9ef0d8ea, compared to commit: 9c2537deb0d2d8dd3f724186df2ab700193eaa26

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
Nexus execute
executeFromExecutor		 +176 ❌
+982 ❌		 +0.36%
+4.96%
Full diff report πŸ‘‡
| Contract | Deployment Cost (+/-) | Method | Min (+/-) | % | Avg (+/-) | % | Median (+/-) | % | Max (+/-) | % | # Calls (+/-) | |:-|-:|:-|-:|-:|-:|-:|-:|-:|-:|-:|-:| | **Nexus** | 5,480,373 (+14,715) | _execute_
_executeFromExecutor_ | 6,356 (+156)
14,719 (+190) | **+2.52%**
**+1.31%** | 49,150 (+176)
20,785 (+982) | **+0.36%**
**+4.96%** | 38,056 (+156)
20,699 (+1,089) | **+0.41%**
**+5.55%** | 143,098 (+120)
27,025 (+1,560) | **+0.08%**
**+6.13%** | 76 (0)
4 (0) |
openzeppelin-code[bot] commented 3 months ago

refactor: Improve error handling in ExecutionHelper contract

Generated at commit: 0734b81b3272441e3385c33b8af9663351de94cd

🚨 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

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/c654f5061ba8ac118b20169a31f048db9ef0d8ea/contracts/contracts/base/RegistryAdapter.sol#L12)) should be constant [base/RegistryAdapter.sol#L12](https://github.com/bcnmy/nexus/blob/c654f5061ba8ac118b20169a31f048db9ef0d8ea/contracts/contracts/base/RegistryAdapter.sol#L12) - [ ] ID-1 [RegistryFactory.threshold]([factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/c654f5061ba8ac118b20169a31f048db9ef0d8ea/contracts/contracts/factory/RegistryFactory.sol#L39)) should be constant [factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/c654f5061ba8ac118b20169a31f048db9ef0d8ea/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 13 lines in your changes missing coverage. Please review.

Project coverage is 71.68%. Comparing base (9c2537d) to head (0734b81). Report is 46 commits behind head on remediations/cantina-spearbit.

Files Patch % Lines
contracts/base/ExecutionHelper.sol 0.00% 13 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## remediations/cantina-spearbit #128 +/- ## ================================================================= - Coverage 72.54% 71.68% -0.87% ================================================================= Files 13 13 Lines 663 671 +8 Branches 151 154 +3 ================================================================= Hits 481 481 - Misses 182 190 +8 ``` | [Files](https://app.codecov.io/gh/bcnmy/nexus/pull/128?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) | Coverage Ξ” | | |---|---|---| | [contracts/base/ExecutionHelper.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/128?src=pr&el=tree&filepath=contracts%2Fbase%2FExecutionHelper.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL2Jhc2UvRXhlY3V0aW9uSGVscGVyLnNvbA==) | `48.03% <0.00%> (-4.09%)` | :arrow_down: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/bcnmy/nexus/pull/128?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/128?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Last update [9c2537d...0734b81](https://app.codecov.io/gh/bcnmy/nexus/pull/128?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

ensuring calls to EOAs are not incorrectly marked as successful.

I notice in case of eoa you're just doing this

mstore(result, 0) // Set result length to 0 mstore(0x40, add(result, 0x20)) // Update free memory pointer

but the other code following can still be executed.

Can you bring auditers attention to this comment again please?

a. Assume all calls to EOA invalid / unsuccessfull b. sucessfull if it's just value transfer > 0 and calldata is 0x

Ensure it returns in such cases and does not go to other control paths.

Write appropriate tests for it

livingrockrises commented 3 months ago

@Aboudjem

livingrockrises commented 3 months ago

inclined to close this.