-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
🔒 H-01 - Ensure msg.value is Forwarded to Prevent Loss of User Funds #113
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #113 +/- ##
===========================================
- Coverage 87.29% 72.18% -15.11%
===========================================
Files 13 13
Lines 559 676 +117
Branches 122 124 +2
===========================================
Hits 488 488
- Misses 69 186 +117
Partials 2 2
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -166,7 +166,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra | |||
(address target, bytes memory data) = abi.decode(innerCall, (address, bytes)); | |||
bool success; | |||
// Perform the call to the target contract with the decoded data. | |||
(success, innerCallRet) = target.call(data); | |||
(success, innerCallRet) = target.call{value: msg.value}(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this data is initialise data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
this is same as please check above link also and make sure everything is covered. and ping the PR link in thread thre. |
We have a response from spearbit. looks good to me but note that step 4) "Other factories don't forward it in INexus(account).initializeAccount(initData)" is still missing. A different bootstrap contract might want to use msg.value? |
function createAccount(
address eoaOwner,
uint256 index,
address[] calldata attesters,
uint8 threshold
) external payable returns (address payable) {
// Compute the actual salt for deterministic deployment
bytes32 actualSalt;
assembly {
let ptr := mload(0x40)
let calldataLength := sub(calldatasize(), 0x04)
mstore(0x40, add(ptr, calldataLength))
calldatacopy(ptr, 0x04, calldataLength)
actualSalt := keccak256(ptr, calldataLength)
}
// Deploy the Nexus contract using the computed salt
(bool alreadyDeployed, address account) = LibClone.createDeterministicERC1967(msg.value, ACCOUNT_IMPLEMENTATION, actualSalt);
// Create the validator configuration using the Bootstrap library
BootstrapConfig memory validator = BootstrapLib.createSingleConfig(K1_VALIDATOR, abi.encodePacked(eoaOwner));
bytes memory initData = BOOTSTRAPPER.getInitNexusWithSingleValidatorCalldata(validator, REGISTRY, attesters, threshold);
// Initialize the account if it was not already deployed
if (!alreadyDeployed) {
INexus(account).initializeAccount{ value: msg.value }(initData);
emit AccountCreated(account, eoaOwner, index);
}
return payable(account);
} The msg.value is already forwarded in the deployment step here, any suggestion? |
🔒 H-03 - Enforce Registry Calls Before Module Setup to Comply with EIP-7484
🔒 H-02 - Prevent Freezing of Funds in Factory Contracts
just make sure all factories forward it |
🤖 Slither Analysis Report 🔎Slither report
# Slither report
_This comment was automatically generated by the GitHub Actions workflow._
THIS CHECKLIST IS NOT COMPLETE. Use
constable-statesImpact: Optimization
|
🔒 H-01 - Ensure msg.value is Forwarded to Prevent Loss of User Funds
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
H-01. User may lose funds when creating Nexus account or executing user operations
Issue: Users may lose funds because
msg.value
is ignored during the creation of a Nexus account or execution of user ops. Any ETH sent with these txs could be lost if not properly handled.Fix: Added handling for
msg.value
to ensure any ETH sent with the tx is forwarded appropriately.Summary of Fixes:
msg.value
in thedeployWithFactory
,executeUserOp
, andfallback
functions to ensure ETH is properly forwarded.