Add controller-specific seizure override for Securitize vaults - #434
Open
dglowinski wants to merge 1 commit into
Open
dglowinski wants to merge 1 commit into
dglowinski wants to merge 1 commit into
Conversation
LeonardEulerXYZ
left a comment
Contributor
There was a problem hiding this comment.
Review — no PR-specific blocking findings
Reviewed head 2c98ce5e068b7985948bfde692737ed1de6e2127 against development (816c5943). This is a review-only COMMENT, not formal approval.
Scope and conclusions
- Reviewed all three changed files: the Securitize vault, its tests, and
docs/securitize-vault.md; traced the inherited ERC20 transfer/update path, EVC routing/authentication, deferred status-check handling, and existing deployment compiler settings. - The bypass requires a nonzero exact match to the source account's sole enabled controller. The legacy overload retains status checking. Previously queued EVC checks are not removed.
- Moving directly through
_transferretains zero-address/insufficient-balance rejection and the Securitize_updateprefix accounting, without modifying allowances. Governor authentication, recipient compliance, pause/freeze restrictions, and reentrancy protection remain on the shared overload. - No dependency, workflow, external-call destination, or privilege expansion beyond the explicitly documented governor seizure override was introduced.
Independent validation
forge test --match-path test/Vault/ERC4626EVCCollateralSecuritize.t.sol -vv: 15 passed, including 1,000 fuzz runs for subaccount/amount/allowance preservation.forge build --offline: passed (compiler warnings).forge fmt --check src/Vault/deployed/ERC4626EVCCollateralSecuritize.sol test/Vault/ERC4626EVCCollateralSecuritize.t.solandgit diff --check origin/development...HEAD: passed.forge test --offline --no-match-path 'test/{HookTarget/HookTargetMarketStatus.t.sol,HookTarget/HookTargetStakeDelegator.t.sol,Liquidator/SBLiquidator.t.sol,OFT/OFTFeeCollectorFork.t.sol,OFT/OFTFeeCollectorGulperFork.t.sol,Swaps/MigrationHelperFork.t.sol,Swaps/Swaps1Inch.sol}': 443 passed, 2 failed across 57 suites. Both failing counterexamples also reproduce on base816c5943withforge test --offline --match-test 'test_GovernanceModifiers|test_redeem_zero_allowance':ERC20SynthTest.test_GovernanceModifiers: expected revert not observed.ERC4626EVCCollateralFreezableStdTest.test_redeem_zero_allowance: assertion failure.
These are baseline failures in this local run, not regressions attributed to this PR. Hosted Foundry CI is successful.
forge build src/VaultFactory/ERC4626EVCCollateralSecuritizeFactory.sol --offline --optimizer-runs 10000 --sizes --out out-size --cache-path cache-size: passed. Vault runtime 19,842 bytes; factory runtime 24,196 bytes, leaving 380 bytes under EIP-170. Creation sizes are 21,699 / 24,387 bytes, respectively.
Documented operational boundaries
Naming a legitimate lending controller can remove debt-backing collateral and impose lender losses; the exact-address match is not evidence of maliciousness. This is the intended, explicitly documented governor capability, not a newly discovered bypass by an unprivileged user. The existing Securitize-specific 10,000-run deployment settings remain important.
Validation used the repository's local Securitize/controller fixtures and pinned EVC implementation. No live deployment, underlying compliance-service integration, or fork test was exercised.
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
A controller whose account status check reverts can block governor seizure. Add
seize(from, to, amount, ignoreController)so the governor can bypass the check for an explicitly named controller.Changes
ignoreControlleraddress. Mismatches, multiple controllers, and the zero address retain normal status checking; the three-argument overload uses the zero address.GovSeizedevents.Test plan
forge fmt --checkon both changed Solidity files andgit diff --check.forge build --offline.forge build src/VaultFactory/ERC4626EVCCollateralSecuritizeFactory.sol --offline --optimizer-runs 10000 --sizesusing separate output and cache paths.Code size
Solidity 0.8.24, Cancun, optimizer enabled with 10,000 runs, matching the Securitize factory settings in
script/interactiveDeployment.shandscript/utils/verifyContracts.sh. EIP-170 permits 24,576 bytes of runtime code.Both runtime sizes remain below the limit: 4,734 bytes of headroom for the vault and 380 bytes for the factory. Both creation bytecodes are below the 49,152-byte EIP-3860 limit before constructor arguments.
The repository's default 20,000-run build produces a 21,750-byte vault and a 26,104-byte factory. The factory exceeds EIP-170 under those defaults, including on the base branch (25,588 bytes); deployment must use the existing Securitize-specific 10,000-run settings.