refactor SetHook preflight validation#725
Conversation
Sync: Ripple(d) 2.4.0
* Add AMM bid/create/deposit/swap/withdraw/vote invariants:
- Deposit, Withdrawal invariants: `sqrt(asset1Balance * asset2Balance) >= LPTokens`.
- Bid: `sqrt(asset1Balance * asset2Balance) > LPTokens` and the pool balances don't change.
- Create: `sqrt(asset1Balance * assetBalance2) == LPTokens`.
- Swap: `asset1BalanceAfter * asset2BalanceAfter >= asset1BalanceBefore * asset2BalanceBefore`
and `LPTokens` don't change.
- Vote: `LPTokens` and pool balances don't change.
- All AMM and swap transactions: amounts and tokens are greater than zero, except on withdrawal if all tokens
are withdrawn.
* Add AMM deposit and withdraw rounding to ensure AMM invariant:
- On deposit, tokens out are rounded downward and deposit amount is rounded upward.
- On withdrawal, tokens in are rounded upward and withdrawal amount is rounded downward.
* Add Order Book Offer invariant to verify consumed amounts. Consumed amounts are less than the offer.
* Fix Bid validation. `AuthAccount` can't have duplicate accounts or the submitter account.
Due to rounding, the LPTokenBalance of the last LP might not match the LP's trustline balance. This was fixed for `AMMWithdraw` in `fixAMMv1_1` by adjusting the LPTokenBalance to be the same as the trustline balance. Since `AMMClawback` is also performing a withdrawal, we need to adjust LPTokenBalance as well in `AMMClawback.` This change includes: 1. Refactored `verifyAndAdjustLPTokenBalance` function in `AMMUtils`, which both`AMMWithdraw` and `AMMClawback` call to adjust LPTokenBalance. 2. Added the unit test `testLastHolderLPTokenBalance` to test the scenario. 3. Modify the existing unit tests for `fixAMMClawbackRounding`.
Sentinel Security Review
Stats: 6 issues · 2 high · 2 medium · 2 low · reviewed 0 files Available CommandsPR-Level (top-level PR comment)
Finding-Level (reply to a Sentinel inline comment)
Sentinel AI |
![sentinel-ai-reviewer[bot]](https://avatars.githubusercontent.com/in/320396?s=60&v=4){kind=small}
| bool | ||
| isHookOnFieldsPresent(STObject const& hookSetObj) | ||
| { | ||
| return hookSetObj.isFieldPresent(sfHookOn) || |
There was a problem hiding this comment.
🟡 MEDIUM · missing_validation
Orphaned HookOnV2 field bypasses validateHookOn and its featureHookOnV2 amendment gate. If only sfHookOnOutgoing is present (without sfHookOnIncoming), isHookOnFieldsPresent returns false, so validateHookOn is never called from validateHookSetFields. For hsoUPDATE/hsoINSTALL operations, no per-operation HookOn validation exists, so the orphan field reaches doApply unvalidated. Critically, the featureHookOnV2 amendment check inside validateHookOn is also bypassed, allowing sfHookOnOutgoing to be applied to a hook even before the HookOnV2 amendment is enabled.
Suggested fix: isHookOnFieldsPresent should also return true when EITHER sfHookOnOutgoing OR sfHookOnIncoming is present (not requiring both), so that validateHookOn can reject the incomplete pair. Alternatively, add an explicit check in validateHookSetFields: if exactly one of sfHookOnOutgoing/sfHookOnIncoming is present without sfHookOn, return false.
| std::variant<bool, std::pair<uint64_t, uint64_t>> | ||
| validateWasmCode(SetHookCtx& ctx, STObject const& hookSetObj) | ||
| { | ||
| Blob hook = hookSetObj.getFieldVL(sfCreateCode); |
There was a problem hiding this comment.
🔵 LOW · missing_validation
getFieldVL(sfCreateCode) called at function entry without isFieldPresent guard. The function relies entirely on callers to ensure sfCreateCode is present before invocation. Currently the sole call site (hsoCREATE at line 640) does check isFieldPresent, but the function is not self-defensive. If any future caller omits the guard, getFieldVL throws, which would be caught as tefEXCEPTION in the try-catch at line 838-852, returning temMALFORMED. However, this masks the actual error.
Suggested fix: Add an isFieldPresent(sfCreateCode) check at the start of validateWasmCode, or document the precondition with an XRPL_VERIFY.
| validateHookAPIVersion(SetHookCtx& ctx, STObject const& hookSetObj) | ||
| { | ||
| auto const apiVersion = hookSetObj.getFieldU16(sfHookApiVersion); | ||
| if (apiVersion != 0) |
There was a problem hiding this comment.
🔵 LOW · missing_validation
getFieldU16(sfHookApiVersion) called without isFieldPresent guard inside the function body. The caller validateHookSetFields checks isFieldPresent before calling, but validateHookAPIVersion itself is not self-defensive. If invoked without the guard, getFieldU16 throws on absent field.
Suggested fix: Add isFieldPresent(sfHookApiVersion) check inside validateHookAPIVersion or use getOptionalU16/~sfHookApiVersion pattern.
| if (flag & STPathElement::typeIssuer) // issuer | ||
| length += 20; | ||
|
|
||
| int next_flag = *(upto + length); |
There was a problem hiding this comment.
🟠 HIGH · memory_corruption
Out-of-bounds read in STI_PATHSET parsing: after incrementing length by up to 60 bytes inside the inner while loop (line 3317-3322 for account+currency+issuer), *(upto + length) is read at line 3324 without verifying upto + length < end. If the serialized pathset data is truncated, this reads memory beyond the input buffer boundary.
Suggested fix: Add bounds check if (upto + length >= end) return Unexpected(pe_unexpected_end); before the read at line 3324.
Backport of XRPLF/rippled#6325. The python version runs ~80x faster.
…gparse" This reverts commit 5c1d7d9.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #725 +/- ##
=====================================
Coverage 77.3% 77.3%
=====================================
Files 837 837
Lines 78575 78584 +9
Branches 11551 11546 -5
=====================================
+ Hits 60748 60758 +10
+ Misses 17817 17816 -1
Partials 10 10
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
6 participants
High Level Overview of Change
Context of Change
Type of Change