feat: gas optimizations to broker logic with fixes

[Ronnieraj37]

📄 Description

A few gas optimizations in src/broker/, plus two correctness fixes I came
across while reading through it.

Scope: LendingBroker.sol, LendingBrokerOperatorLib.sol, RateCalculator.sol.
No storage slots reordered; one new variable is appended (see Upgrade safety).

🧠 Rationale

Two correctness issues worth fixing regardless of gas:

• Fee-on-transfer accounting. _pullPayment credited the requested amount
  rather than the amount received, so a fee-taking loan token would under-collect
  on every repay. Now credits the balance delta (as _borrowFromMoolah already
  does), with an InsufficientAmount guard in repayAll. No-op for zero-fee
  tokens; relevant given USDT's owner-settable fee.
• Wrong revert error. An inconsistent-input check reverted InvalidMarketId;
  changed to a new InconsistentInput.

The gas changes remove work from the hot paths (accrual, fixed repay, borrow):
an unbounded term scan becomes O(1), a full array-to-memory copy becomes a
storage scan, and a same-block accrual short-circuits instead of rewriting
storage.

Honest trade-off: on the current test suite (~1 fixed position, 3 terms) the
bundle is near break-even — the fee-on-transfer safety reads (change 1) offset
the wins, and changes 6 & 7 only pull ahead at scale (more positions/terms). I
verified deployment state on BSC mainnet (maxFixedLoanPositions = 100); the
per-change effects are in the summary below so maintainers can weigh each.

Upgrade safety

termIndexPlusOne is appended after the existing V2 storage — no slot moves.
Proxies that already have terms need a one-time backfill, run atomically with
the upgrade:

upgradeToAndCall(newImpl, abi.encodeCall(LendingBroker.initializeTermIndex, ()))

initializeTermIndex is a reinitializer(2). Skipping it on a broker with
existing terms leaves the index empty and _getTermById reverts TermNotFound.
Fresh deployments (and brokers with no terms) don't need it.

🧪 Example / Testing

• forge build — clean.
• forge test --match-path 'test/broker/*' — passes.
• No existing tests changed.

I can add dedicated tests for the new paths (term-index add/remove with
swap-and-pop re-pointing, the backfill reinitializer, a fee-on-transfer repay
case) if maintainers want them in this PR.

🧬 Changes Summary

Notable changes:

• Fee-on-transfer accounting in repay paths (fix) — credit the measured
  balance delta, not the requested amount; repayAll gains an
  InsufficientAmount guard.
• Inconsistent liquidation input reverts InconsistentInput (fix) — was
  InvalidMarketId. Note: this changes a revert selector.
• RateCalculator cleanup (gas) — shared accrual helper, same-block
  short-circuit, and drop a redundant SLOAD in getRate (~1.75k vs ~6.9k on
  same-block accrual; −288 bytes).
• RateCalculator.registerBroker: struct literal → direct field assignment
  (gas) — minor runtime saving + smaller bytecode (one-time admin call).
• getLiquidationWhitelist → EnumerableSet.values() (gas) — ~18%
  cheaper, scales with set size.
• Fixed-position lookup memory → storage scan (gas) — up to ~8.4k saved
  at 10 positions (break-even ~2).
• O(1) term lookup via appended termIndexPlusOne mapping (gas) —
  ~1.7k/borrow at 3 terms, scales with term count.

Notes / decisions

Things I deliberately left as-is, with the reasoning:

• Fixed positions stay an array (not a mapping). A parallel id-list measured
  worse — it adds a cold SSTORE to every borrow, and the whole-collection
  operations (delete, bulk-assign in liquidation/refinance) get more expensive.
• Duplicated helpers left duplicated. The fixed-position helpers
  (_getFixedPositionByPosId, _removeFixedPositionByPosId,
  _updateFixedPosition) exist in both LendingBroker and the operator library.
  Internal library functions are inlined, so sharing them saves nothing; making
  them public (delegatecall) measured +245 gas/call. Keeping the
  duplication is the cheaper option, at the cost of keeping the two copies in
  sync.
• RateConfig left unpacked. Packing would save on the once-per-broker
  registerBroker but cost on every accrual (shared-slot read-modify-write), and
  accrual is the hot path.
• No inline assembly. There is more gas available (tight SLOAD/SSTORE control
  in the loops, manual bounds-check elision), but for audited financial contracts
  the readability/auditability cost isn't worth it in a general contribution. If
  a specific hot path is worth it — the liquidation cascade is the best candidate
  — I'd propose that as its own narrowly-scoped PR.

Happy to split the correctness fixes and the optimizations into separate PRs if
that's easier to review — and glad to take a look at other parts of the codebase
too if the maintainers would find it useful.

[hashdit-bot]

Pull Request Review

This Solidity upgradeable lending-broker PR improves fee-on-transfer repayment accounting, introduces a dedicated liquidation input error, and adds gas optimizations for rate accrual, whitelist retrieval, and fixed-position scans. It also appends an indexed fixed-term lookup mapping with a one-time reinitializer for existing proxies and maintains that index during term additions, updates, and removals.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.

---

Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.