feat: add access control protection suite#57
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.
mateo-mro
left a comment
mateo-mro
left a comment
There was a problem hiding this comment.
PR description is empty. For 4k lines across three 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.