fix(staking): S-1 LDV controller hijack + S-2 adapter swap-under-load#127
Merged
Merged
Conversation
S-1 (LiquidDelegationVault.requestRedeem): An authorized operator (ERC-7540 `_operators[owner][caller]`) could file a redemption with `controller != owner`, then drive `redeem` to route assets anywhere. Now an operator-driven request must set `controller == owner`; owners filing on their own behalf may still pick any controller (the legit ERC-7540 aggregator-vault use case). S-2 (StakingAssetsFacet.registerAdapter / removeAdapter): Switching adapters while the asset has live deposits silently strands balances in the old adapter or double-counts them in the new one. Both paths now revert with `AdapterChangeWhileDepositsExist(token, deposits)` when `currentDeposits != 0`. The M-8 migration framework (`startAdapterMigration` -> `completeAdapterMigration`) remains the intended drain path for live assets. The new error is exposed on `IMultiAssetDelegation` so the Rust bindings decode the revert. Tests: - test_Vault_OperatorCannotPickArbitraryController_S1 - test_Vault_OwnerMayPickArbitraryController_S1 (preserves legit case) - test_registerAdapter_blockedWhileDepositsExist_S2 - test_removeAdapter_blockedWhileDepositsExist_S2 - test_registerAdapter_allowedWhenDepositsAreZero_S2 Bindings: ABI regenerated; new custom error decoded by alloy. Version bump deferred to whichever Round 4 PR lands last so we don't race four PRs onto v0.15.0.
3 tasks
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
Two surgical staking-layer fixes from the Round 4 architectural audit:
controller != owner, then driveredeemto route the owner's assets to any receiver. Operator-filed requests now requirecontroller == owner. Owner-filed requests may still pick any controller — that's a legitimate ERC-7540 aggregator pattern.registerAdapter/removeAdaptercalls silently corrupted accounting when the asset had live deposits — balances either stranded in the old adapter or double-counted in the new one. Both paths now revert withAdapterChangeWhileDepositsExist(token, deposits)whencurrentDeposits != 0. The M-8 migration framework (startAdapterMigration→completeAdapterMigration) remains the intended drain path.What changed
src/staking/LiquidDelegationVault.sol— operator-vs-controller gate inrequestRedeem.src/facets/staking/StakingAssetsFacet.sol—currentDeposits == 0guard onregisterAdapter/removeAdapterplus new custom error.src/interfaces/IMultiAssetDelegation.sol— exposes the new error so Rust bindings can decode the revert.test/staking/LiquidDelegationTest.t.sol— two S-1 regressions (operator must setcontroller == owner; owner can still delegate to a separate controller).test/security/AdapterChangeWhileDepositsExistTest.t.sol— three S-2 regressions (register-blocked-while-deposits, remove-blocked-while-deposits, both allowed at zero deposits).bindings/— ABI regenerated; new error wired into alloy decoder.Why these are surgical
Bindings version
ABI changes only (one new custom error). Version bump deferred to whichever Round 4 PR lands last, to avoid racing four PRs onto v0.15.0.
Test plan
forge test --match-path test/security/AdapterChangeWhileDepositsExistTest.t.sol -vv— 3/3 passforge test --match-path test/staking/LiquidDelegationTest.t.sol --match-test "Operator|Owner"— 5/5 passforge test --match-path "test/staking/*.t.sol"— 262/262 passforge test --match-path "test/security/*.t.sol"— 25/25 passFOUNDRY_PROFILE=local_build forge buildcleancargo build --release -p tnt-core-bindingscleanforge test(delegated)