Conversation
![sentinel-ai-reviewer[bot]](https://avatars.githubusercontent.com/in/320396?s=60&v=4){kind=small}
| * registration on tesSUCCESS. | ||
| */ | ||
| [[nodiscard]] TER | ||
| couponSettle( |
There was a problem hiding this comment.
The accrual computation constructs additionAmt via STAmount{couponAmount.asset(), addition} (CouponHelpers.cpp:68) before the canAdd overflow guard on the next line has any chance to run. When count * RegisteredUnits * CouponAmount exceeds the asset's representable range — which is fully attacker-controlled via registration units and elapsed intervals — the STAmount constructor throws std::overflow_error or std::runtime_error. None of the three transactors that call couponSettle (CouponClaim, CouponRegister, CouponUnregister) catch this exception, so it bubbles to the outer applySteps handler and becomes tefEXCEPTION. Because sfLastSettledTime is written on lines 77-78, after the throw, it never advances — and since CouponUnregister::doApply calls couponSettle before releasing locked units, the holder can neither claim nor exit, permanently locking their registered bond units. Fix: bound-check the Number multiplication before constructing the STAmount, or wrap the construction in a try/catch that translates to tecPRECISION_LOSS.
| */ | ||
| [[nodiscard]] TER | ||
| couponLockPreclaim( | ||
| ReadView const& view, |
There was a problem hiding this comment.
The IOU specialization of the lock preclaim uses canAdd(spendable, units) to guard the locking operation, but the actual operation is a debit — directSendNoFee subtracts units from the holder's trust-line balance. For IOU amounts, canSubtract always returns true (subtraction is always representable), while canAdd performs a round-trip precision check that can return false for large balance/unit combinations where addition precision would be lossy. The mismatch means valid lock operations on large IOU balances can be spuriously rejected with tecPRECISION_LOSS. This is the same incorrect-direction precision check confirmed in escrowLockPreclaimHelper<Issue> in EscrowHelpers.h. Replace canAdd(spendable, units) with canSubtract(spendable, units) to match the actual arithmetic direction.
| TYPED_SFIELD(sfFirstCouponTime, UINT32, 76) | ||
| TYPED_SFIELD(sfCallNoticePeriod, UINT32, 77) | ||
| TYPED_SFIELD(sfEarliestCallTime, UINT32, 78) | ||
| TYPED_SFIELD(sfLastSettledTime, UINT32, 79) |
There was a problem hiding this comment.
sfEarliestCallTime accepts 0 and any past timestamp — there is no check that it is non-zero, greater than the current ledger close time, or at or after sfFirstCouponTime. The call-protection enforcement in CouponScheduleSet compares newExpiration < sfEarliestCallTime, so when sfEarliestCallTime is 0 that comparison is trivially false against any real timestamp, rendering the call-protection window completely non-functional. An issuer can create a callable bond with sfFirstCouponTime one year out and sfEarliestCallTime = 0, attract holders who lock their units via CouponRegister, then immediately submit CouponScheduleSet to expire the bond one notice-period later — before a single coupon is ever paid — while investors cannot recover their principal until they submit CouponUnregister themselves. Add a preflight rejection for sfEarliestCallTime == 0 and sfEarliestCallTime < sfFirstCouponTime, and a preclaim rejection for sfEarliestCallTime <= parentCloseTime, consistent with how sfFirstCouponTime is already validated against the live ledger time.
Suggested fix
In CouponScheduleCreate::preflight: (1) reject sfEarliestCallTime == 0 with temMALFORMED; (2) reject sfEarliestCallTime < sfFirstCouponTime with temBAD_EXPIRATION. In preclaim: (3) reject sfEarliestCallTime <= parentCloseTime.time_since_epoch().count() with tecEXPIRED, mirroring the existing sfFirstCouponTime check.
| TYPED_SFIELD(sfFirstCouponTime, UINT32, 76) | ||
| TYPED_SFIELD(sfCallNoticePeriod, UINT32, 77) | ||
| TYPED_SFIELD(sfEarliestCallTime, UINT32, 78) | ||
| TYPED_SFIELD(sfLastSettledTime, UINT32, 79) |
There was a problem hiding this comment.
sfEarliestCallTime accepts 0 and any past timestamp — there is no check that it is non-zero, greater than the current ledger close time, or at or after sfFirstCouponTime. The call-protection enforcement in CouponScheduleSet compares newExpiration < sfEarliestCallTime, so when sfEarliestCallTime is 0 that comparison is trivially false against any real timestamp, rendering the call-protection window completely non-functional. An issuer can create a callable bond with sfFirstCouponTime one year out and sfEarliestCallTime = 0, attract holders who lock their units via CouponRegister, then immediately submit CouponScheduleSet to expire the bond one notice-period later — before a single coupon is ever paid — while investors cannot recover their principal until they submit CouponUnregister themselves. Add a preflight rejection for sfEarliestCallTime == 0 and sfEarliestCallTime < sfFirstCouponTime, and a preclaim rejection for sfEarliestCallTime <= parentCloseTime, consistent with how sfFirstCouponTime is already validated against the live ledger time.
Suggested fix
In CouponScheduleCreate::preflight: (1) reject sfEarliestCallTime == 0 with temMALFORMED; (2) reject sfEarliestCallTime < sfFirstCouponTime with temBAD_EXPIRATION. In preclaim: (3) reject sfEarliestCallTime <= parentCloseTime.time_since_epoch().count() with tecEXPIRED, mirroring the existing sfFirstCouponTime check.
|
No failures yet, but checks are still running: |
6 similar comments
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
80d4b17 to
5af42fc
Compare
dangell7
added
merge-order-06
1 participant
Adds
featureCouponPayments.Spec: https://gist.github.com/dangell7/98dbeffc32baa3d0b59a5b3be557ced3