fix(WrappedM): address remaining ChainSecurity audit issues#134
Merged
Conversation
…ngFor An earner could point their claim recipient at a frozen address so that the permissionless stopEarningFor() reverts inside _claim() -> _transfer() -> _revertIfFrozen(recipient_), keeping the account earning indefinitely and defeating the deauthorization mechanism. Both stopEarningFor() overloads now fall back to skipTransfer when the resolved claim recipient is frozen, mirroring the _beforeFreeze path: the yield is crystallized onto the account's own balance instead of being routed to the frozen recipient, so deauthorization always succeeds.
The WrappedMTokenMigratorV1 migration corrupts accounting if an address appears twice in the earners array, since the first pass overwrites the lastIndex slot with the computed principal. ListOfEarnersToMigrate now requires strictly ascending addresses, enforcing sorted order, uniqueness and non-zero in a single O(n) pass without the gas cost of an O(n^2) check. Integration test fixtures sort the in-memory earners copy before deploying the migrator, mirroring the production generation script.
Make the earners list generation safe to rely on for the v1->v2 migration: - Paginate holder fetches so earners are never silently truncated by the `first` cap, with a max-pages guard against a broken `skip`. - Build the whole file in memory and write once after every network succeeds; abort without writing on any error, and exit non-zero, so a partial run never mixes fresh and stale lists. - Throw on duplicate or zero earner addresses instead of relying solely on the on-chain check. - Fix the GraphQL response typing (drop the unsound cast), add a request timeout, centralize the chain enum, and drop the unused balance field.
LCOV of commit
|
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
* fix(deploy): sort earners before migrator construction * refactor(test): pass earners storage array directly to migrator deploy Drop the manual storage-to-memory copy loops in the integration test fixtures; assigning or passing the storage array to a memory parameter already performs the copy. --------- Co-authored-by: Pierrick Turelier <pierrick@turelier.com>
toninorair
reviewed
Jun 24, 2026
| const REQUEST_TIMEOUT_MS = 30_000; | ||
|
|
||
| const NETWORKS = [ | ||
| "arbitrum", |
Contributor
There was a problem hiding this comment.
is this the final list? Some networks like Monad are missing — is it due to a lack of wM earners on them?
Member
Author
There was a problem hiding this comment.
No it's not the final one.
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
3 participants
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.
Issues:
An earner could point their claim recipient at a frozen address so that the permissionless
stopEarningFor()reverts inside_claim()->_transfer()->_revertIfFrozen(recipient_), keeping the account earning indefinitely and defeating the deauthorization mechanism.Both
stopEarningFor()overloads now fall back toskipTransferwhen the resolved claim recipient is frozen, mirroring the_beforeFreezepath: the yield is crystallized onto the account's own balance instead of being routed to the frozen recipient, so deauthorization always succeeds.The WrappedMTokenMigratorV1 migration corrupts accounting if an address appears twice in the earners array, since the first pass overwrites the
lastIndexslot with the computed principal.ListOfEarnersToMigrate now requires strictly ascending addresses, enforcing sorted order, uniqueness and non-zero.
Improvement: