[Certora] lib/bundler assets/debts reachability#916
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb5264a55a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eabec97f3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| uint256 debtBefore = midnight.debtOf(id, onBehalf); | ||
| require U > 0 && U <= debtBefore, "target units within current debt"; | ||
|
|
||
| require midnight.isAuthorized(onBehalf, currentContract), "bundler must be authorized on Midnight for repay"; |
There was a problem hiding this comment.
Constrain the bundler caller authorization
This only assumes the bundler is authorized for the inner Midnight.repay, but repayAndWithdrawCollateral first requires the external caller to be onBehalf or authorized by onBehalf. For any e.msg.sender that fails that outer check, the plain CVL call below silently prunes the revert path, so the rule can pass without covering those callers. Add require onBehalf == e.msg.sender || midnight.isAuthorized(onBehalf, e.msg.sender) (or switch to @withrevert and assert the expected revert behavior).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
that's the idea of CVL
There was a problem hiding this comment.
codex is suggesting to add a require, but do we even need the one that is currently written ?
bhargavbh
left a comment
There was a problem hiding this comment.
very nice roundtrip properties to have for bundler's liveness guarantees.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed13dd96a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bhargavbh
left a comment
There was a problem hiding this comment.
LGTM. Please update the README with the rules description.
Co-authored-by: Bhargav Bhatt <40268131+bhargavbh@users.noreply.github.com> Signed-off-by: lilCertora <lilian@certora.com>
| import {Offer} from "../../src/interfaces/IMidnight.sol"; | ||
| import {TakeAmountsLib} from "../../src/periphery/TakeAmountsLib.sol"; | ||
|
|
||
| /// @dev Thin harness exposing TakeAmountsLib's `internal` library functions to CVL. |
There was a problem hiding this comment.
| /// @dev Thin harness exposing TakeAmountsLib's `internal` library functions to CVL. |
| // Deterministic market id (same pattern as Midnight.spec / TakeAmountsLibInvertibility.spec). | ||
| function IdLib.toId(Midnight.Market memory market, uint256, address) internal returns (bytes32) => summaryToId(market); | ||
|
|
||
| // Deterministic floor division; must not call MulDiv (it delegates back to UtilsLib.mulDivDown). |
There was a problem hiding this comment.
| // Deterministic floor division; must not call MulDiv (it delegates back to UtilsLib.mulDivDown). | |
| // Deterministic mulDivDown. |
I don't understand the rest
| } | ||
|
|
||
| function summaryMulDivDown(uint256 a, uint256 b, uint256 d) returns uint256 { | ||
| if (d == 0) { |
There was a problem hiding this comment.
| if (d == 0) { | |
| bool overflow; | |
| if (d == 0 || overflow) { |
| if (d == 0) { | ||
| revert(); | ||
| } | ||
| return require_uint256((to_mathint(a) * to_mathint(b)) / to_mathint(d)); |
There was a problem hiding this comment.
| return require_uint256((to_mathint(a) * to_mathint(b)) / to_mathint(d)); | |
| return require_uint256(a * b / d); |
| // Deterministic floor division; must not call MulDiv (it delegates back to UtilsLib.mulDivDown). | ||
| function UtilsLib.mulDivDown(uint256 x, uint256 y, uint256 d) internal returns (uint256) => summaryMulDivDown(x, y, d); | ||
|
|
||
| // Token / permit functions are irrelevant to the units formula but NONDET prevent havoc calls. |
There was a problem hiding this comment.
| // Token / permit functions are irrelevant to the units formula but NONDET prevent havoc calls. | |
| // Token / permit functions are irrelevant to the units formula, use a NONDET summary to prevent calls that havoc Midnight. |
|
|
||
| /// MULDIV GHOST SUMMARIES (mirrors AccrualRounding.spec) /// | ||
|
|
||
| persistent ghost summaryMulDivDownM(mathint, mathint, mathint) returns mathint { |
There was a problem hiding this comment.
| persistent ghost summaryMulDivDownM(mathint, mathint, mathint) returns mathint { | |
| persistent ghost ghostMulDivDown(mathint, mathint, mathint) returns mathint { |
| /* proved in mulDivUpRoundsUp */ | ||
| definition axiomUpLowerBound(mathint a, mathint b, mathint d) returns bool = 0 <= a && 0 <= b && 0 < d => summaryMulDivUpM(a, b, d) * d >= a * b; | ||
|
|
||
| /* proved in mulDivUpUpperBound */ | ||
| definition axiomUpUpperBound(mathint a, mathint b, mathint d) returns bool = 0 <= a && 0 <= b && 0 < d => summaryMulDivUpM(a, b, d) * d <= a * b + d - 1; | ||
|
|
||
| /* proved in mulDivDownRoundsDown */ | ||
| definition axiomDownUpperBound(mathint a, mathint b, mathint d) returns bool = 0 <= a && 0 <= b && 0 < d => summaryMulDivDownM(a, b, d) * d <= a * b; | ||
|
|
||
| /* proved in mulDivDownTightBound */ | ||
| definition axiomDownTightBound(mathint a, mathint b, mathint d) returns bool = 0 <= a && 0 <= b && 0 < d => (summaryMulDivDownM(a, b, d) + 1) * d > a * b; |
There was a problem hiding this comment.
let's put them as axioms in the ghost variables, that avoids defining the requireMulDivAxioms function
| requireMulDivAxioms(); | ||
|
|
||
| bytes32 id = summaryToId(offer.market); | ||
| require tickSpacing(id) > 0, "market is already created"; |
There was a problem hiding this comment.
| require tickSpacing(id) > 0, "market is already created"; |
it should not be needed because settlementFee() reverts if the market is not created
| requireMulDivAxioms(); | ||
|
|
||
| bytes32 id = summaryToId(offer.market); | ||
| require tickSpacing(id) > 0, "market is already created"; |
There was a problem hiding this comment.
| require tickSpacing(id) > 0, "market is already created"; |
| // Mirror the library's sellerPrice computation in CVL to express the WAD precondition. | ||
| uint256 offerPrice = summaryTickToPrice(offer.tick); | ||
| uint256 timeToMaturity = e.block.timestamp <= offer.market.maturity ? assert_uint256(offer.market.maturity - e.block.timestamp) : 0; | ||
| uint256 fee = settlementFee(id, timeToMaturity); | ||
| require !offer.buy || fee <= offerPrice, "library reverts otherwise"; | ||
|
|
There was a problem hiding this comment.
| // Mirror the library's sellerPrice computation in CVL to express the WAD precondition. | |
| uint256 offerPrice = summaryTickToPrice(offer.tick); | |
| uint256 timeToMaturity = e.block.timestamp <= offer.market.maturity ? assert_uint256(offer.market.maturity - e.block.timestamp) : 0; | |
| uint256 fee = settlementFee(id, timeToMaturity); | |
| require !offer.buy || fee <= offerPrice, "library reverts otherwise"; |
same reasoning
No description provided.