feat: perpetuals protection suite#55
Conversation
makemake-kbo
requested review from
fredo,
lean-apple and
mateo-mro
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
makemake-kbo
force-pushed
the
makemake/eng-2355-featcredible-std-add-protection-suite-for-dexes
branch
from
9f560af to
10067e0
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
mateo-mro
left a comment
mateo-mro
left a comment
There was a problem hiding this comment.
PR description is empty. Also: title says "perpetuals protection suite" but the diff ships the full lending suite (ILendingProtectionSuite, LendingBaseAssertion, Aave v3 Horizon example) as well. Either this stacks on another unmerged branch that should be linked, or the title/scope need to match — right now these lending files also appear in #57, so whichever lands first will churn the other.
For a ~4k-line addition across two new suites we need a writeup: which invariants each suite enforces, why the step-oriented interface shape, which exploits the example assertions are modelled against, and how detection was validated end-to-end.
Main blocker is that the Denaria tests don't actually exercise the assertion — the author even says so in a comment. Rest of the concerns inline.
| // compilation and the assertion registration path. The full behavioral test | ||
| // requires the Credible Layer runtime with fork-aware state reads. | ||
| vm.prank(alice); | ||
| pair.removeLiquidity(1_000 * TOKEN_UNIT, 10 * TOKEN_UNIT, 0, ""); |
There was a problem hiding this comment.
Name says Reverts, there's no vm.expectRevert, and the comment block above admits the mock can't drive per-fork state so the check never fires. This passes unconditionally — tells us nothing about whether EQUITY_CONSERVATION works. Either wire up a fixture that produces a real pre/post delta, or move it to a proper integration test under the Credible runtime. Delete before merge if neither.
| vm.prank(attacker); | ||
| pair.realizePnL(""); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same story as the stale-accounting test — no vm.expectRevert, the mock doesn't distinguish forks, assertion never runs. If we're claiming this catches the Denaria hack we need a runtime-level test that actually reverts on the exploit path. Right now it's a smoke test dressed up as a regression, which is worse than no test because it gives false confidence.
| /// @notice Verifies DenariaOperationSafetyAssertion deploys and exposes the right selector. | ||
| function testAssertionDeployment() public { | ||
| new DenariaOperationSafetyAssertion(address(pair), address(vault)); | ||
| assertEq( |
There was a problem hiding this comment.
Both sides of this assertEq are the same expression — always passes. If pinning the selector is the intent, compare against DenariaOperationSafetyAssertion.assertOperationSafety.selector.
| uint256 globalLiqStable = | ||
| _readUintAt(PERP_PAIR, abi.encodeCall(IDenariaPerpPairLike.globalLiquidityStable, ()), beforeFork); | ||
|
|
||
| bool assetAtCap = beforeMetrics.lpAssetBalance == globalLiqAsset && globalLiqAsset > 0; |
There was a problem hiding this comment.
Exact equality as the fingerprint is too sharp both ways. It false-positives on a legitimate solo LP (early pool life, quiet market, post-migration) — honest calls will revert for those users. It also misses any wrap that lands a wei short of the cap because of a rounding buffer upstream. Catch the overflow at its source (the int256→uint256 cast site) or widen to >= cap with an explicit solo-LP carve-out. As-is this is both too strict and not strict enough.
| /// @author Phylax Systems | ||
| /// @notice Compatibility alias preserving the old generic Aave v3 suite name. | ||
| /// @dev The implementation is Horizon-specific and derived from the local Horizon repository. | ||
| contract AaveV3ProtectionSuite is AaveV3HorizonProtectionSuite { |
There was a problem hiding this comment.
All three compat aliases (AaveV3ProtectionSuite, AaveV3OperationSafetyAssertion, AaveV3PostOperationSolvencyAssertion) are introduced in the same PR that creates the file. There's nothing prior to be compatible with — these are dead code on arrival. Drop them; we can always add shims later if an external consumer materialises.
| /// @title DenariaPostMutationRiskAssertion | ||
| /// @author Phylax Systems | ||
| /// @notice Compatibility alias for users who only care about the post-mutation risk gate. | ||
| contract DenariaPostMutationRiskAssertion is DenariaOperationSafetyAssertion { |
There was a problem hiding this comment.
Same as on the Aave file — alias with no prior consumer. Delete.
2 participants
No description provided.