Repository navigation
[midnight adapter] round net rate computations once - #1213
devin-ai-integration[bot] wants to merge 4 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@devin-ai-integration[bot] evaluate how strong the griefing can be without this pr and what we gain with it; use high but realistic rates and tokens that make the case harder (but no lower than 8 decimals for a very high value token and no lower than 6 overall) |
|
@adhusson Short version: without this PR the griefing is weak. It can't be done for profit and it can only pull interest forward, never take more than the remaining interest. The one place it bites is small positions in an 8-decimal, high-value token. There, a few thousand to tens of thousands of takes can push Method
Cost per take: about 256k gas for the first take and about 141k gas for each extra take in the same transaction. At 1 gwei with ETH at $4,000 that is about $0.56 per take. On an L2 at 0.005 gwei it is about $0.003. Effect per 0-unit take on main:
The worst cost-to-effect ratio is about 2:1 on a cheap L2 (WBTC at 1 wei per take), versus about 460:1 on mainnet. The attacker also does not receive the moved value. It only lifts the share price earlier and slows accrual later, so the only way to benefit is to hold shares and exit before maturity, and even then only for your share of the vault. The vault's Driving
Once What the PR gains
My conclusion is that this is not a meaningful exploit on main; the PR mainly closes the free, unlimited repetition and stops tiny positions from being zeroed. Whether that is worth the formula change is your and @peyha's call. |
|
Verdict: without this PR, anyone can drift Per-take drift on
With this PR:
So the PR closes the free path (no offer at a lower rate needed, nothing owed) and removes the 1-wei rounding term. On large long positions it doesn't bound the drift. That needs finer Replay scriptWAD=10**18; YEAR=365*86400
def V(N,r,t): return N*WAD//(WAD+r*t) # realAssets term
def buy_main(N,r,t,b,p):
N2=N+b; A=-(-(N2-b)*WAD//(WAD+r*t))+p # mulDivUp
return N2,(N2-A)*WAD//(A*t)
def buy_pr(N,r,t,b,p):
N2=N+b; d=WAD+r*t
return N2,((N2-b)*(d-WAD)+(b-p)*d)*WAD//(((N2-b)*WAD+p*d)*t)
# 0-unit take: b=p=0; tiny take of u units at offer price px: b=u, p=u*px//WAD
# excess per take = (V(after) - V(before) - sum(p)) / takes |
|
what would be more complex single rounding like here or moving from wad to ray precision |
|
@adhusson Moving Complexity
Effect: I replayed the old formula (round-up then round-down) at both precisions with the same setup as my previous comment. The table shows value moved into
The drift has two parts:
This PR removes both parts for 0-unit takes and drops part 1 entirely. Only part 2 remains, at most one rate unit per real trade. Adding RAY on top would shrink that remaining part too, but at the storage and interface cost above. |
|
is there a simpler way of doing single rounding / increasing precision |
|
@adhusson Yes, there's a simpler form, and I've pushed it in 40c2bc0. Both functions now keep main's exact shape, // onBuy
uint256 discount = WAD + _marketData.netRate * ttm;
uint256 amortizedValueTimesDiscount = (newNetCredit - boughtNetCredit) * WAD + paidAssets * discount;
_marketData.netRate = (newNetCredit * discount - amortizedValueTimesDiscount)
.mulDivDown(WAD, amortizedValueTimesDiscount * ttm).toUint64();
// onSell, shortfall branch (taken iff soldNetCredit * WAD > sellerAssets * discount)
uint256 amortizedValueTimesDiscount = (newNetCredit + soldNetCredit) * WAD - sellerAssets * discount;
require(amortizedValueTimesDiscount <= newNetCredit * discount, ShortfallTooHigh()); // main: amortizedValue <= newNetCredit
_marketData.netRate = (newNetCredit * discount - amortizedValueTimesDiscount)
.mulDivDown(WAD, amortizedValueTimesDiscount * ttm).toUint64();This gives the same numbers as the previous commit, since I don't see anything simpler on the precision side:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ade1a5cc20
ℹ️ 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".
| _marketData.netRate = (newNetCredit * discount - amortizedValueTimesDiscount) | ||
| .mulDivDown(WAD, amortizedValueTimesDiscount * ttm) |
There was a problem hiding this comment.
Use overflow-safe arithmetic for the rescaled rate
For large but permitted positions and rates, scaling both operands by discount makes this calculation overflow even when the resulting rate fits in uint72. For example, netCredit may approach uint128.max, netRate may approach uint72.max, and ttm may approach uint32.max; then both the numerator multiplication inside mulDivDown and amortizedValueTimesDiscount * ttm exceed uint256, whereas the previous unscaled calculation did not. This makes otherwise valid subsequent buys revert; the mirrored shortfall-sale calculation has the same issue and should also use overflow-safe scaling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the numerator overflow on ade1a5cc with an actual Midnight take (no storage injection): a first buy of 1e31 asset units at price 1e11 (WAD, i.e. 1e-7) and 30-day maturity creates netCredit = 1e38 and netRate = 3858024305555555555. A subsequent zero-unit buy panics with 0x11 in the scaled numerator multiplication. The previous unscaled formula evaluates successfully, returning 3858024305555555554, within uint72.
These are extreme amounts/rates, but the position is reachable. A temporary Foundry reproduction passed; it was not added to the branch. The denominator claim is not established by this reproduction. Leaving the thread open and the implementation unchanged pending approval to extend this update with an overflow-safe fix.
MidnightAdapter’s two-stage rounding can lower
netRateeven on zero-unit buys, recognizing unearned interest early. Compute buy and shortfall-sale rates with a single round-down so zero-unit buys leave the rate unchanged. The final rate rounding on real trades remains; already-deployed adapters are unchanged.Refs PRO-300. Also fixes Sherlock #24.
Link to Devin session: https://app.devin.ai/sessions/d5ecf0f28e49407492d9770342ea6e17
Open in Devin Desktop: https://app.devin.ai/desktop/session/d5ecf0f28e49407492d9770342ea6e17?variant=devin