Merged
Conversation
… 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.
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.
…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.
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.
…lidator 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.
Addresses audit finding #3 (repeated validation). The zero deposit check in _onSingleExecutorExecute is already performed by RecurringAllowance.useLimit() which reverts with ZeroValue.
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.
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.
amiecorso
changed the title
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
1 participant
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
Addresses all 9 findings from the second audit (Red Swan / Giovanni Di Siena).
Findings addressed
Low severity
_uninstall/_uninstallForReplacenow capture revert data and emitPolicyUninstallHookReverted(policyId, account, policy, revertData).MorphoLendPolicyandMorphoLoanProtectionPolicynow prependapprove(spender, 0)before the actual approval, handling USDT-style tokens.setPolicyManagerambiguities and executor signature replay: RemovedsetPolicyManagerentirely;policyManageris nowimmutable.setPolicyManagerinsufficient for mass migration: Resolved by removingsetPolicyManager.Informational
PublicERC6492Validatorconstructor now checks for code at the Solady verifier address.MorphoLendPolicyvalidation: Removed redundantZeroAmountcheck (already covered byRecurringAllowance.ZeroValue)._installand_replacenow call_requirePersistentCodeto reject empty or EIP-7702-only addresses._installand_installForReplacenow revert withInvalidValidityWindowwhenvalidAfter >= validUntil(both non-zero).setPolicyManagermissing same-value check: Resolved by removingsetPolicyManager.