Root + Impact

Description

• redeem(token, amountOfAssetToken) is guarded by the revertIfZero(amountOfAssetToken) modifier. Solidity modifiers evaluate using the caller's raw, original argument, before the function body runs.

• Inside the function body, if (amountOfAssetToken == type(uint256).max) { amountOfAssetToken = assetToken.balanceOf(msg.sender); } remaps the "redeem everything" sentinel value to the caller's real balance - but this remapping happens after the modifier has already evaluated and passed.

• For a zero-balance account: redeem(token, 0) correctly reverts with ThunderLoan__CantBeZero(), because the modifier sees a literal 0. But redeem(token, type(uint256).max) on the exact same zero-balance account does not revert - the modifier sees a non-zero type(uint256).max and passes, then the body remaps the amount down to 0, and burn(0)/transferUnderlyingTo(0) both execute successfully, emitting Redeemed(account, token, 0, 0).

• Two calls that are semantically identical ("there is nothing to redeem") produce two different contract behaviors - a revert vs. a silent successful no-op - purely because of which literal the caller happened to write.

• No funds move in either case, and no economic exploit exists - this is a behavioral/state-handling inconsistency, not a fund-safety issue, matching CodeHawks' Low-impact definition ("funds not at risk, but function behaves incorrectly / state handled improperly").

• src/upgradedProtocol/ThunderLoanUpgraded.sol::redeem() contains the identical logic and modifier ordering, so it is affected the same way.

Risk

Likelihood:

• Reason 1 // Requires the caller to use the specific sentinel value type(uint256).max (a "redeem all" convention, e.g. what a wallet's "Max" button would send) on an account that happens to hold a zero balance - a specific but realistic combination of parameter and account state, not an arbitrary/always-on trigger.

• Reason 2 // No attacker or special timing is needed - any ordinary integrator or wallet UI that uses the type(uint256).max "redeem all" convention will hit this silently on any zero-balance account.

Impact:

• Impact 1 // No funds move and no value is lost or gained by anyone - protocol reserves and the caller's balances are unchanged before and after, confirmed directly by the PoC.

• Impact 2 // Breaks the expectation that "nothing to redeem" behaves consistently: a wallet's "Redeem All" button on an empty position would silently succeed with zero effect and a Redeemed(...,0,0) event, instead of giving the caller a clear revert/error signal that there was nothing to redeem.

Proof of Concept

Ran with forge test --match-path "test/PoC_11.t.sol" -vv: all 3 tests pass. test_redeemZero_reverts confirms the baseline: redeem(tokenA, 0) on a zero-balance account reverts with ThunderLoan__CantBeZero. test_redeemMax_onZeroBalance_silentlyNoOps_insteadOfReverting confirms redeem(tokenA, type(uint256).max) on the exact same zero-balance account does not revert, and precisely emits Redeemed(victim, tokenA, 0, 0) - proving execution proceeds into the function body rather than reverting. test_noOp_causesNoFundMovement_protocolReservesUnchanged confirms the protocol's tokenA reserves are bit-for-bit identical before and after the no-op call, proving this is a pure behavioral inconsistency with no fund-safety impact. Full regression suite (20 tests, including a pre-existing repo test independently pointing at the same root cause) passing, no regressions.

Recommended Mitigation

Move the zero-amount check to after the sentinel value has been resolved to a real balance, instead of gating on the caller's raw argument via the revertIfZero modifier. This way, both redeem(token, 0) and redeem(token, type(uint256).max) on a zero-balance account consistently revert with the same error, regardless of which literal the caller used. Apply the identical fix to src/upgradedProtocol/ThunderLoanUpgraded.sol::redeem(), since it shares the same modifier-ordering issue.