Root + Impact

Description

• getCalculatedFee() performs two sequential floor (integer) divisions instead of multiplying all factors together and dividing once: valueOfBorrowedToken = (amount * price) / s_feePrecision (floor #1), then fee = (valueOfBorrowedToken * s_flashLoanFee) / s_feePrecision (floor #2) - instead of the mathematically equivalent single-division form fee_ideal = (amount * price * s_flashLoanFee) / s_feePrecision^2.

• The source even carries a //slither-disable-next-line divide-before-multiply comment right above this code, showing the divide-before-multiply pattern was already flagged by static analysis and the warning was suppressed rather than fixed.

• Because floor(floor(a/b)*c/d) <= floor(a*c/(b*d)) always holds, this two-step formula can only match or undercharge relative to the mathematically correct single-division fee - never overcharge. A 256-case fuzz test across random (amount, price) pairs confirms onChainFee <= idealFee with zero counterexamples, proving this is a structural, deterministic bias, not an occasional rounding coincidence.

• This fee is fed directly into assetToken.updateExchangeRate(), so the shortfall is not a pure-function curiosity - it is exactly what LPs are credited through the live deposit()/flashloan() path, on every single flash loan.

• src/upgradedProtocol/ThunderLoanUpgraded.sol::getCalculatedFee() contains the identical two-step formula, so the issue is carried into the planned upgrade unchanged.

Risk

Likelihood:

• Reason 1 // Fires deterministically on every deposit()/flashloan() call whenever the oracle price is not an exact multiple of s_feePrecision (1e18) - which is the normal case for essentially any real price feed, requiring no attacker action, special timing, or unusual parameters.

• Reason 2 // Confirmed structural (not coincidental) via 256 randomized (amount, price) fuzz runs, all showing the same one-directional undercharge.

Impact:

• Impact 1 // Funds are not at direct risk of theft or insolvency from this issue alone - it is a real, measurable, but small-magnitude computational shortfall in LP fee revenue (independent of the separate oracle-manipulation and WETH-value-unit-mismatch issues, which cause much larger, more direct financial impact).

• Impact 2 // Because the shortfall is bounded by rounding/precision-dust magnitude rather than scaling proportionally with loan size, the direct per-transaction loss is small, but it is a real, structural, and permanent under-collection versus the mathematically correct fee.

Proof of Concept

Ran with forge test --match-path "test/PoC_9.t.sol" -vv: all 3 tests pass. test_getCalculatedFee_TwoStepFlooringUnderchargesByOneWei reproduces a concrete example (amount=774894766378519688946574, price=6673886014218663356): the on-chain fee (15514678031474523216741) matches the manually-replicated two-step formula exactly, and is exactly 1 wei less than the mathematically ideal single-division fee (15514678031474523216742). testFuzz_TwoStepFeeNeverExceedsIdealFee runs 256 randomized (amount, price) pairs and confirms onChainFee <= idealFee holds in every case with zero counterexamples - proving the undercharge direction is a mathematical guarantee of the code's structure. test_RealFlashLoan_LPsReceiveTwoStepFeeNotIdealFee runs the exact scenario through a real deposit() -> flashloan() -> repay() path and confirms the AssetToken exchange rate is updated using the (slightly short) two-step fee, not the ideal fee - proving this is the actual value LPs are paid, not just a pure-function artifact. Full regression suite (20 tests across 5 files) passing, no regressions. A custom ConfigurablePool/ConfigurablePoolFactory was needed because the repo's own MockTSwapPool is hard-coded to return exactly 1e18, which makes the first division exact and hides this bug entirely - a non-1:1 price is required to expose it, matching realistic production price feeds.

Recommended Mitigation

Merge the two sequential divisions into a single division after all multiplicative factors are combined: fee = (amount * price * s_flashLoanFee) / (s_feePrecision * s_feePrecision). Since amount * price * s_flashLoanFee can overflow uint256 for very large amount/price values, prefer a 512-bit-intermediate-precision mulDiv implementation (e.g. OpenZeppelin's Math.mulDiv) over the raw triple product: fee = Math.mulDiv(amount * price, s_flashLoanFee, s_feePrecision * s_feePrecision) (or equivalent chained mulDiv calls) to get single-division precision without overflow risk. Apply the same fix to both src/protocol/ThunderLoan.sol and src/upgradedProtocol/ThunderLoanUpgraded.sol, and add a regression test comparing the two-step and single-division results across randomized (amount, price) pairs to prevent this from silently regressing.