Fix/finding #18 Prevent Memory Overwrite in withdrawDepositTo Function

[Aboudjem]

This PR addresses the memory overwrite issue identified in finding #18. The problem arises in the withdrawDepositTo function where the amount argument overwrites part of the free memory pointer, leading to potential out-of-gas reverts due to quadratic memory expansion costs. Additionally, this could prevent the actual revert return data from bubbling up.

Changes Made:

1. Updated the withdrawDepositTo function to store the free memory pointer at the beginning of the assembly block.
2. Modified the return data handling to use the stored free memory pointer, ensuring accurate return data copying and reverting.

[openzeppelin-code]

Fix/finding #18 Prevent Memory Overwrite in withdrawDepositTo Function

Generated at commit: ec1d3b027dae7523af68da33326df06c649d14cd

🚨 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]

Codecov Report

Attention: Patch coverage is 61.53846% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.60%. Comparing base (80d3913) to head (902c284).
Report is 38 commits behind head on remediations/cantina-spearbit.

Files	Patch %	Lines
contracts/base/BaseAccount.sol	0.00%	8 Missing ⚠️
contracts/base/ExecutionHelper.sol	87.50%	2 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           remediations/cantina-spearbit     #125      +/-   ##
=================================================================
+ Coverage                          72.19%   75.60%   +3.41%     
=================================================================
  Files                                 13       13              
  Lines                                694      664      -30     
  Branches                             150      153       +3     
=================================================================
+ Hits                                 501      502       +1     
+ Misses                               193      162      -31     

Files	Coverage Δ
contracts/Nexus.sol	63.06% <100.00%> (ø)
contracts/base/ModuleManager.sol	83.73% <100.00%> (+0.09%)	⬆️
contracts/base/ExecutionHelper.sol	58.16% <87.50%> (+1.78%)	⬆️
contracts/base/BaseAccount.sol	59.45% <0.00%> (-1.66%)	⬇️

---

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 0070740...902c284. Read the comment docs.

[livingrockrises]

the linked issue is wrong ser. #18 goes to github issue number 18

[livingrockrises]

shouldn't you/we open an issue on solady repo and validate and discuss this there as well?

[livingrockrises]

review i

[github-actions]

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary
🟡 - locked-ether (1 results) (Medium)

• constable-states (2 results) (Optimization)

locked-ether

🟡 Impact: Medium
🔴 Confidence: High

• ID-0
  Contract locking ether found:
  Contract Bootstrap has payable functions:
  • ModuleManager.receive()
  • ModuleManager.fallback()
    But does not have a function to withdraw the ether

utils/RegistryBootstrap.sol#L33-L165

constable-states

Impact: Optimization
🔴 Confidence: High

• ID-1
  RegistryAdapter.registry should be constant

base/RegistryAdapter.sol#L12

• ID-2
  RegistryFactory.threshold should be constant

factory/RegistryFactory.sol#L39

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