fix(recovery): bind a signed deadline into AssetRecoveryModule (I-04 parity with SafeAssetRecoveryModule)#165
Open
seongyun-ko wants to merge 1 commit into
Open
fix(recovery): bind a signed deadline into AssetRecoveryModule (I-04 parity with SafeAssetRecoveryModule)#165seongyun-ko wants to merge 1 commit into
seongyun-ko wants to merge 1 commit into
Conversation
Port the audit I-04 mitigation from SafeAssetRecoveryModule to its LayerZero sibling AssetRecoveryModule, which lacked it. Both recovery modules perform a full-balance sweep and do not sign an amount (the destination sweeps whatever balance is present at delivery). SafeAssetRecoveryModule already binds a signed deadline so a relayer cannot stash a valid owner signature and replay it against a future, larger deposit of the same token. AssetRecoveryModule bound no deadline, so a valid signature never expired: only the per-safe nonce made it single-use, but it stayed replayable in time until consumed. Changes: - add a signed deadline param to recover(); revert RecoveryExpired past it - bind deadline into the recovery digest (mirrors SafeAssetRecoveryModule) - add RecoveryExpired error + deadline to IAssetRecoveryModule - update the LZ recover callers (unit + E2E suites) and add a test_recover_revertsIfExpired regression All 29 recovery/E2E tests pass (TEST_CHAIN=10).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![certora-run[bot]](https://avatars.githubusercontent.com/in/1083145?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verification Results
- Group ID: 6a5992d8-9ef9-4f65-b1a2-6d33f233e88a
- Commit: debb682
| Job | Result | VERIFIED |
|---|---|---|
| EtherFiSafe.… | ✅ | 14 |
| CashModuleCo… | ✅ | 2 |
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
AssetRecoveryModule(the LayerZero recovery variant) did not bind a signed deadline, while its siblingSafeAssetRecoveryModuledoes (audit finding I-04). This PR ports that mitigation so the two recovery modules are consistent.Why
Both recovery modules sweep the full token balance and do not sign an amount — the destination sweeps whatever balance is present at delivery.
SafeAssetRecoveryModulebinds a signeddeadline(I-04) precisely so a relayer cannot stash a valid owner signature and later replay it against a future, larger deposit of the same token.AssetRecoveryModulebound no deadline, so a valid recovery signature never expired: the per-safe nonce made it single-use, but it remained replayable in time until consumed. Since the recovery caller is untrusted (EtherFi submits the owner-authorized recovery), a signed authorization for a small stuck balance could be held and dispatched later against a much larger balance. The recipient is bound in the digest (so this is not fund redirection), but the authorization should be time-bounded like the sibling module.Changes
recover()takes adeadlineand revertsRecoveryExpiredonceblock.timestamp > deadlinedeadlineis bound into the recovery digest (mirrorsSafeAssetRecoveryModule)IAssetRecoveryModule: addRecoveryExpirederror +deadlineparamrecovercallers in the unit + E2E suitestest_recover_revertsIfExpiredregression (parity with the safe variant)Test plan
Note for reviewers
recover()'s signature changes (addsdeadline). Any off-chain owner-signing tooling that builds the recovery digest must adddeadlineto the ABI-encoded digest, in the same position as the on-chain_verifyRecoverySignatures.Note
Medium Risk
Changes fund-recovery authorization semantics and breaks the recover ABI/digest for integrators, but the guard is narrow and mirrors an existing sibling module pattern.
Overview
Adds a signed
deadlineto cross-chainAssetRecoveryModule.recover, matchingSafeAssetRecoveryModuleand closing audit I-04: destination recovery sweeps the full token balance with no signed amount, so a relayer could otherwise hold a valid owner signature until a larger balance arrived.recovernow takesuint256 deadline, revertsRecoveryExpiredwhenblock.timestamp > deadline, and includesdeadlinein the EIP-style recovery digest in_verifyRecoverySignatures.IAssetRecoveryModulegains the error and updated signature.Unit and E2E tests pass
deadline(defaulttype(uint256).max);test_recover_revertsIfExpiredcovers post-deadline rejection. Breaking: off-chain digest/signing must ABI-encodedeadlinein the same slot as on-chain.Reviewed by Cursor Bugbot for commit debb682. Bugbot is set up for automated code reviews on this repo. Configure here.