Skip to content

add storage diff script and test it #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

add storage diff script and test it #131

wants to merge 3 commits into from

Conversation

mihailo-maksa
Copy link
Contributor

In this PR, I added the storage-diff.sh script to help us identify any possible errors in a storage layout for the new implementation contracts in an automated way. Also added a mock contract to test out the script, as well as the generalized script for deploying new implementation contracts as needed.

The storage-diff.sh script I added is inspired by and based on the similar script used by EigenLayer.

@mihailo-maksa mihailo-maksa requested a review from auroter April 7, 2025 12:51
@github-actions
Copy link

github-actions bot commented Apr 7, 2025

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

⚠️ Could not parse test results

🔒 Security Analysis

⚠️ Found 3 High and 1 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call
reentrancy-eth

Impact: Reentrancy in EthMultiVault.batchDeposit(address,uint256[],uint256[]) (src/EthMultiVault.sol#1219-1252): External calls: - _transferFeesToProtocolMultisig(protocolFee) (src/EthMultiVault.sol#1248) - (success,None) = address(generalConfig.protocolMultisig).call{value: value}() (src/EthMultiVault.sol#1418) State variables written after the call(s): - shares[i] = _deposit(receiver,termIds[i],userDepositAfterprotocolFee) (src/EthMultiVault.sol#1247) - vaults[id].totalAssets = totalAssets (src/EthMultiVault.sol#1759) - vaults[id].balanceOf[to] += amount (src/EthMultiVault.sol#1714) - vaults[id].totalShares = totalShares (src/EthMultiVault.sol#1760) EthMultiVault.vaults (src/EthMultiVault.sol#97) can be used in cross function reentrancies: - EthMultiVault.convertToAssets(uint256,uint256) (src/EthMultiVault.sol#2166-2170) - EthMultiVault.convertToShares(uint256,uint256) (src/EthMultiVault.sol#2131-2135) - EthMultiVault.currentSharePrice(uint256) (src/EthMultiVault.sol#2067-2073) - EthMultiVault.getDepositSharesAndFees(uint256,uint256) (src/EthMultiVault.sol#1859-1886) - EthMultiVault.getRedeemAssetsAndFees(uint256,uint256) (src/EthMultiVault.sol#1926-1960) - EthMultiVault.getVaultStateForUser(uint256,address) (src/EthMultiVault.sol#2303-2307) - EthMultiVault.maxRedeem(address,uint256) (src/EthMultiVault.sol#2105-2108) - EthMultiVault.vaults (src/EthMultiVault.sol#97)

Affected Files:

  • src/EthMultiVault.sol
View Detailed Findings
  • src/EthMultiVault.sol:1219 in batchDeposit
  • src/EthMultiVault.sol:1269 in batchDepositCurve

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2400-2412) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2403)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2400 in _validateTimelock

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

⚠️ No gas snapshot generated

@github-actions
Copy link

github-actions bot commented Apr 7, 2025

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

⚠️ Could not parse test results

🔒 Security Analysis

⚠️ Found 3 High and 1 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call
reentrancy-eth

Impact: Reentrancy in EthMultiVault.batchDeposit(address,uint256[],uint256[]) (src/EthMultiVault.sol#1219-1252): External calls: - _transferFeesToProtocolMultisig(protocolFee) (src/EthMultiVault.sol#1248) - (success,None) = address(generalConfig.protocolMultisig).call{value: value}() (src/EthMultiVault.sol#1418) State variables written after the call(s): - shares[i] = _deposit(receiver,termIds[i],userDepositAfterprotocolFee) (src/EthMultiVault.sol#1247) - vaults[id].totalAssets = totalAssets (src/EthMultiVault.sol#1759) - vaults[id].balanceOf[to] += amount (src/EthMultiVault.sol#1714) - vaults[id].totalShares = totalShares (src/EthMultiVault.sol#1760) EthMultiVault.vaults (src/EthMultiVault.sol#97) can be used in cross function reentrancies: - EthMultiVault.convertToAssets(uint256,uint256) (src/EthMultiVault.sol#2166-2170) - EthMultiVault.convertToShares(uint256,uint256) (src/EthMultiVault.sol#2131-2135) - EthMultiVault.currentSharePrice(uint256) (src/EthMultiVault.sol#2067-2073) - EthMultiVault.getDepositSharesAndFees(uint256,uint256) (src/EthMultiVault.sol#1859-1886) - EthMultiVault.getRedeemAssetsAndFees(uint256,uint256) (src/EthMultiVault.sol#1926-1960) - EthMultiVault.getVaultStateForUser(uint256,address) (src/EthMultiVault.sol#2303-2307) - EthMultiVault.maxRedeem(address,uint256) (src/EthMultiVault.sol#2105-2108) - EthMultiVault.vaults (src/EthMultiVault.sol#97)

Affected Files:

  • src/EthMultiVault.sol
View Detailed Findings
  • src/EthMultiVault.sol:1219 in batchDeposit
  • src/EthMultiVault.sol:1269 in batchDepositCurve

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2400-2412) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2403)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2400 in _validateTimelock

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

⚠️ No gas snapshot generated

auroter
auroter requested changes Apr 9, 2025
View reviewed changes
Copy link
Member

@auroter auroter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super cool, but let's omit the output of the script from the repo by adding it to gitignore, and add this to the github workflow instead to include it in the generated docs when we merge to main.

@github-actions
Copy link

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

⚠️ Could not parse test results

🔒 Security Analysis

⚠️ Found 3 High and 1 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call
reentrancy-eth

Impact: Reentrancy in EthMultiVault.batchDeposit(address,uint256[],uint256[]) (src/EthMultiVault.sol#1244-1277): External calls: - _transferFeesToProtocolMultisig(protocolFee) (src/EthMultiVault.sol#1273) - (success,None) = address(generalConfig.protocolMultisig).call{value: value}() (src/EthMultiVault.sol#1443) State variables written after the call(s): - shares[i] = _deposit(receiver,termIds[i],userDepositAfterprotocolFee) (src/EthMultiVault.sol#1272) - vaults[id].totalAssets = totalAssets (src/EthMultiVault.sol#1784) - vaults[id].balanceOf[to] += amount (src/EthMultiVault.sol#1739) - vaults[id].totalShares = totalShares (src/EthMultiVault.sol#1785) EthMultiVault.vaults (src/EthMultiVault.sol#97) can be used in cross function reentrancies: - EthMultiVault.convertToAssets(uint256,uint256) (src/EthMultiVault.sol#2191-2195) - EthMultiVault.convertToShares(uint256,uint256) (src/EthMultiVault.sol#2156-2160) - EthMultiVault.currentSharePrice(uint256) (src/EthMultiVault.sol#2092-2098) - EthMultiVault.getDepositSharesAndFees(uint256,uint256) (src/EthMultiVault.sol#1884-1911) - EthMultiVault.getRedeemAssetsAndFees(uint256,uint256) (src/EthMultiVault.sol#1951-1985) - EthMultiVault.getVaultStateForUser(uint256,address) (src/EthMultiVault.sol#2328-2332) - EthMultiVault.maxRedeem(address,uint256) (src/EthMultiVault.sol#2130-2133) - EthMultiVault.vaults (src/EthMultiVault.sol#97)

Affected Files:

  • src/EthMultiVault.sol
View Detailed Findings
  • src/EthMultiVault.sol:1244 in batchDeposit
  • src/EthMultiVault.sol:1294 in batchDepositCurve

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2425-2437) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2428)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2425 in _validateTimelock

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

⚠️ No gas snapshot generated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@auroter auroter auroter requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mihailo-maksa @auroter