[StepSecurity] Apply security best practices#2
Open
stepsecurity-app[bot] wants to merge 1 commit intomainfrom
Open
[StepSecurity] Apply security best practices#2stepsecurity-app[bot] wants to merge 1 commit intomainfrom
stepsecurity-app[bot] wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: StepSecurity Bot <bot@stepsecurity.io>
amiecorso
added a commit
that referenced
this pull request
Mar 27, 2026
…llowed Addresses audit finding #2 (swallowed policy hook reverts emit no events). All 4 catch blocks in _uninstall and _uninstallForReplace now capture revert data and emit PolicyUninstallHookReverted(policyId, account, policy, revertData), giving on-chain observers visibility into forced uninstalls where policy cleanup logic did not run. Also fixes a pre-existing fuzz collision in test_reverts_whenOldPolicyNotInstalled where oldPolicyId could equal newPolicyId, hitting InvalidPayload before PolicyNotInstalled.
amiecorso
added a commit
that referenced
this pull request
Mar 28, 2026
* Gate _execute on non-empty executionData in install/replace signature flows Move the install-only vs install+execute decision into the manager: installWithSignature and replaceWithSignature now only call _execute when executionData is non-empty. This removes the requirement for policies to implement a sentinel check on empty executionData, and ensures every onExecute call represents a genuinely intended execution. Also moves the validity window check before onExecute in _execute, since the deferred-check workaround is no longer needed — install-only flows never reach _execute at all. Removes sentinel checks from SingleExecutorAuthorizedPolicy and CallForwardingPolicy. Updates NatSpec throughout. * Remove setPolicyManager and make policyManager immutable Addresses audit findings #7 (signature replay on manager change), #8 (missing same-value check), and #9 (insufficient for migration). Rather than patching setPolicyManager, remove it entirely — live migration via this mechanism is too fragile and we will use other approaches. With no mutation site, policyManager becomes immutable, saving gas on every onlyPolicyManager modifier check. * Emit PolicyUninstallHookReverted when policy teardown reverts are swallowed Addresses audit finding #2 (swallowed policy hook reverts emit no events). All 4 catch blocks in _uninstall and _uninstallForReplace now capture revert data and emit PolicyUninstallHookReverted(policyId, account, policy, revertData), giving on-chain observers visibility into forced uninstalls where policy cleanup logic did not run. Also fixes a pre-existing fuzz collision in test_reverts_whenOldPolicyNotInstalled where oldPolicyId could equal newPolicyId, hitting InvalidPayload before PolicyNotInstalled. * Add zero-approve before approve in Morpho policy execution blueprints Addresses audit finding #6 (non-standard ERC-20 approve semantics). Tokens like USDT revert on approve(spender, nonZero) when the existing allowance is non-zero. Both MorphoLendPolicy and MorphoLoanProtectionPolicy now prepend an approve(spender, 0) call before the actual approval, preventing stuck policies when the account has an outstanding allowance. Adds ApprovalResetToken mock and tests for both policies proving deposits/top-ups succeed with a pre-existing non-zero allowance. * Add constructor check for Solady ERC-6492 verifier in PublicERC6492Validator Addresses audit finding #1 (ERC-6492 validation depends on Solady's pre-deployed verifier). Reverts at deploy time with ERC6492VerifierNotDeployed() if the verifier at 0x0000bc370E4DC924F427d84e2f4B9Ec81626ba7E has no code, preventing silent signature validation failures on chains where it is absent. * Remove redundant zero-amount check in MorphoLendPolicy Addresses audit finding #3 (repeated validation). The zero deposit check in _onSingleExecutorExecute is already performed by RecurringAllowance.useLimit() which reverts with ZeroValue. * Check policy addresses for persistent code at install time Addresses audit finding #4 (policies not checked for persistent code upon installation). Adds _requirePersistentCode helper that rejects addresses with no code or only an EIP-7702 delegation prefix. Called in _install and _replace (for the new policy) to fail fast rather than leaving accounts with an unexecutable binding. * Reject bindings with impossible validity windows Addresses audit finding #5 (unusable bindings with validAfter >= validUntil). Both _install and _installForReplace now revert with InvalidValidityWindow when both bounds are non-zero and validAfter >= validUntil, preventing permanently unusable bindings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request has been generated by StepSecurity as part of your enterprise subscription to ensure compliance with recommended security best practices. Please review and merge the pull request to apply these security enhancements.
Security Fixes
Harden Runner
Harden-Runner is an open-source security agent for the GitHub-hosted runner to prevent software supply chain attacks. It prevents exfiltration of credentials, detects tampering of source code during build, and enables running jobs without sudo access.
Pinned Dependencies
Pinning GitHub Actions to specific versions or commit SHAs ensures that your workflows remain consistent and secure.
Unpinned actions can lead to unexpected changes or vulnerabilities caused by upstream updates.
Feedback
For bug reports, feature requests, and general feedback; please create an issue in step-security/secure-repo or contact us via our website.