Thunder Loan
AI First Flight #7
Beginner FriendlyFoundryDeFiOracle
EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

setAllowedToken(token, false) followed by re-enabling permanently strands existing LPs' deposited funds in an orphaned AssetToken

comphygxp

Root + Impact

Description

  • setAllowedToken(token, false) simply does delete s_tokenToAssetToken[token]. redeem()'s revertIfNotAllowedToken modifier checks exactly this mapping via isAllowedToken(), so the moment an owner disables a token for any ordinary operational reason (e.g. its underlying TSwap pool needs to be temporarily taken offline), every LP who already deposited into that token is immediately unable to redeem() - AssetToken exposes no way to withdraw funds outside of ThunderLoan (transferUnderlyingTo/burn are onlyThunderLoan).

  • It gets worse on re-enable: because the mapping entry was deleted, calling setAllowedToken(token, true) again passes the AlreadyAllowed check and the protocol news a brand-new AssetToken instance, repointing the mapping to it. The old AssetToken still physically holds every previous LP's real underlying tokens and share balances, but from this point on every ThunderLoan function (deposit/redeem/flashloan/repay) only ever operates through the mapping, which now points exclusively at the new instance. The old instance and everything it holds becomes permanently unreachable through any ThunderLoan function - there is no migration path.

  • This is not an "admin fat-fingered a parameter" self-inflicted-harm case: the owner is using setAllowedToken exactly as designed (a normal disable/re-enable governance action), and the harmed party is a third-party LP, not the owner. src/upgradedProtocol/ThunderLoanUpgraded.sol (explicitly in scope as the upcoming upgrade) has the identical setAllowedToken logic with no fix and no migration/rescue function, so this is not a known, soon-to-be-patched issue - it carries forward into the next version.

function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) {
if (allowed) {
if (address(s_tokenToAssetToken[token]) != address(0)) {
revert ThunderLoan__AlreadyAllowed();
}
@> string memory name = string.concat("ThunderLoan ", IERC20Metadata(address(token)).name());
string memory symbol = string.concat("tl", IERC20Metadata(address(token)).symbol());
@> AssetToken assetToken = new AssetToken(address(this), token, name, symbol);
s_tokenToAssetToken[token] = assetToken;
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
} else {
AssetToken assetToken = s_tokenToAssetToken[token];
@> delete s_tokenToAssetToken[token];
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
}
}

Risk

Likelihood:

  • Reason 1 // Requires two conditions together: the owner role calling setAllowedToken(token, false), and that token already having LP deposits - a specific but realistic and protocol-anticipated operational scenario (e.g. temporarily pulling a token whose price source misbehaves), not an arbitrary-user-anytime trigger, hence Medium rather than High.

  • Reason 2 // No attacker or special timing/race condition is needed beyond the owner performing the disable/re-enable sequence the contract itself explicitly supports.

Impact:

  • Impact 1 // LPs' already-deposited real underlying assets become immediately unreachable through any ThunderLoan function the moment the token is disabled - direct fund freeze.

  • Impact 2 // Re-enabling the token does not restore access; it orphans the old AssetToken and all funds inside it permanently, with no migration or rescue path in either ThunderLoan.sol or the upcoming ThunderLoanUpgraded.sol.

Proof of Concept

Ran with forge test --match-path "test/PoC_2.t.sol" -vvv: [PASS] testDisablingTokenPermanentlyLocksExistingLPFunds() (gas: 4348221). Sequence: owner allow-lists tokenA, creating AssetToken V1; an honest LP deposits 1000e18 tokenA into V1 and receives V1 shares (asserted). Owner then calls setAllowedToken(tokenA, false) as an ordinary operation; the LP's redeem() call reverts with ThunderLoan__NotAllowedToken and the 1000e18 remains stuck in V1 (asserted). Owner re-enables tokenA: a brand-new AssetToken V2 is confirmed minted (V2 != V1), and getAssetFromToken(tokenA) now points only to V2 (asserted). V2 has zero record of the LP's deposit (balanceOf == 0, tokenA.balanceOf(V2) == 0), while the LP's V1 shares remain unchanged at 1000e18 - but redeem() now only operates on V2, so it reverts again, and the funds stay permanently stranded in the now-unreachable V1 (all asserted). Full regression suite: 19/19 passing, no regressions.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { Test, console } from "forge-std/Test.sol";
import { BaseTest, ThunderLoan } from "./unit/BaseTest.t.sol";
import { AssetToken } from "../src/protocol/AssetToken.sol";
​
contract PoC_2_SetAllowedTokenStrandsFunds is BaseTest {
address liquidityProvider = address(0xABCD);
uint256 constant DEPOSIT_AMOUNT = 1_000e18;
​
function setUp() public override {
super.setUp();
}
​
function testDisablingTokenPermanentlyLocksExistingLPFunds() public {
vm.prank(thunderLoan.owner());
AssetToken assetTokenV1 = thunderLoan.setAllowedToken(tokenA, true);
​
tokenA.mint(liquidityProvider, DEPOSIT_AMOUNT);
vm.startPrank(liquidityProvider);
tokenA.approve(address(thunderLoan), DEPOSIT_AMOUNT);
thunderLoan.deposit(tokenA, DEPOSIT_AMOUNT);
vm.stopPrank();
​
assertEq(tokenA.balanceOf(address(assetTokenV1)), DEPOSIT_AMOUNT, "V1 should hold LP's real funds");
assertEq(assetTokenV1.balanceOf(liquidityProvider), DEPOSIT_AMOUNT, "LP should hold V1 shares");
​
vm.prank(thunderLoan.owner());
thunderLoan.setAllowedToken(tokenA, false);
assertFalse(thunderLoan.isAllowedToken(tokenA), "token should now be disabled");
​
vm.startPrank(liquidityProvider);
vm.expectRevert(abi.encodeWithSelector(ThunderLoan.ThunderLoan__NotAllowedToken.selector, address(tokenA)));
thunderLoan.redeem(tokenA, type(uint256).max);
vm.stopPrank();
assertEq(tokenA.balanceOf(address(assetTokenV1)), DEPOSIT_AMOUNT, "funds stuck in V1");
​
vm.prank(thunderLoan.owner());
AssetToken assetTokenV2 = thunderLoan.setAllowedToken(tokenA, true);
​
assertTrue(address(assetTokenV2) != address(assetTokenV1), "re-enable mints a fresh AssetToken");
assertEq(address(thunderLoan.getAssetFromToken(tokenA)), address(assetTokenV2), "mapping now points to V2 only");
​
assertEq(assetTokenV2.balanceOf(liquidityProvider), 0, "V2 has no record of LP's old deposit");
assertEq(tokenA.balanceOf(address(assetTokenV2)), 0, "V2 holds none of LP's real funds");
assertEq(assetTokenV1.balanceOf(liquidityProvider), DEPOSIT_AMOUNT, "LP still nominally owns V1 shares");
​
vm.prank(liquidityProvider);
vm.expectRevert();
thunderLoan.redeem(tokenA, DEPOSIT_AMOUNT);
​
assertEq(tokenA.balanceOf(address(assetTokenV1)), DEPOSIT_AMOUNT, "funds permanently stranded in orphaned V1");
}
}

Recommended Mitigation

function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) {
if (allowed) {
- if (address(s_tokenToAssetToken[token]) != address(0)) {
+ if (s_tokenIsAllowed[token]) {
revert ThunderLoan__AlreadyAllowed();
}
+ if (address(s_tokenToAssetToken[token]) == address(0)) {
string memory name = string.concat("ThunderLoan ", IERC20Metadata(address(token)).name());
string memory symbol = string.concat("tl", IERC20Metadata(address(token)).symbol());
AssetToken assetToken = new AssetToken(address(this), token, name, symbol);
s_tokenToAssetToken[token] = assetToken;
+ }
+ s_tokenIsAllowed[token] = true;
- emit AllowedTokenSet(token, assetToken, allowed);
- return assetToken;
+ emit AllowedTokenSet(token, s_tokenToAssetToken[token], allowed);
+ return s_tokenToAssetToken[token];
} else {
- AssetToken assetToken = s_tokenToAssetToken[token];
- delete s_tokenToAssetToken[token];
+ s_tokenIsAllowed[token] = false;
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
}
}

Separate the "is this token currently allowed for new deposits/flashloans" flag from the AssetToken instance's lifecycle: keep s_tokenToAssetToken[token] intact once created (never delete it), and gate deposit()/flashloan() on a dedicated s_tokenIsAllowed[token] bool via isAllowedToken(). redeem() should NOT be gated by this flag at all - existing LPs must always be able to redeem from whatever AssetToken they already hold shares in, regardless of whether the token is currently paused for new activity. This guarantees deposits made before a disable remain 100% redeemable afterward, and re-enabling never orphans a prior instance. Add a regression/invariant test asserting that funds deposited before a disable are still fully redeemable after any disable/re-enable sequence.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[M-01] 'ThunderLoan::setAllowedToken' can permanently lock liquidity providers out from redeeming their tokens

## Description If the 'ThunderLoan::setAllowedToken' function is called with the intention of setting an allowed token to false and thus deleting the assetToken to token mapping; nobody would be able to redeem funds of that token in the 'ThunderLoan::redeem' function and thus have them locked away without access. ## Vulnerability Details If the owner sets an allowed token to false, this deletes the mapping of the asset token to that ERC20. If this is done, and a liquidity provider has already deposited ERC20 tokens of that type, then the liquidity provider will not be able to redeem them in the 'ThunderLoan::redeem' function. ```solidity function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) { if (allowed) { if (address(s_tokenToAssetToken[token]) != address(0)) { revert ThunderLoan__AlreadyAllowed(); } string memory name = string.concat("ThunderLoan ", IERC20Metadata(address(token)).name()); string memory symbol = string.concat("tl", IERC20Metadata(address(token)).symbol()); AssetToken assetToken = new AssetToken(address(this), token, name, symbol); s_tokenToAssetToken[token] = assetToken; emit AllowedTokenSet(token, assetToken, allowed); return assetToken; } else { AssetToken assetToken = s_tokenToAssetToken[token]; @> delete s_tokenToAssetToken[token]; emit AllowedTokenSet(token, assetToken, allowed); return assetToken; } } ``` ```solidity function redeem( IERC20 token, uint256 amountOfAssetToken ) external revertIfZero(amountOfAssetToken) @> revertIfNotAllowedToken(token) { AssetToken assetToken = s_tokenToAssetToken[token]; uint256 exchangeRate = assetToken.getExchangeRate(); if (amountOfAssetToken == type(uint256).max) { amountOfAssetToken = assetToken.balanceOf(msg.sender); } uint256 amountUnderlying = (amountOfAssetToken * exchangeRate) / assetToken.EXCHANGE_RATE_PRECISION(); emit Redeemed(msg.sender, token, amountOfAssetToken, amountUnderlying); assetToken.burn(msg.sender, amountOfAssetToken); assetToken.transferUnderlyingTo(msg.sender, amountUnderlying); } ``` ## Impact The below test passes with a ThunderLoan\_\_NotAllowedToken error. Proving that a liquidity provider cannot redeem their deposited tokens if the setAllowedToken is set to false, Locking them out of their tokens. ```solidity function testCannotRedeemNonAllowedTokenAfterDepositingToken() public { vm.prank(thunderLoan.owner()); AssetToken assetToken = thunderLoan.setAllowedToken(tokenA, true); tokenA.mint(liquidityProvider, AMOUNT); vm.startPrank(liquidityProvider); tokenA.approve(address(thunderLoan), AMOUNT); thunderLoan.deposit(tokenA, AMOUNT); vm.stopPrank(); vm.prank(thunderLoan.owner()); thunderLoan.setAllowedToken(tokenA, false); vm.expectRevert(abi.encodeWithSelector(ThunderLoan.ThunderLoan__NotAllowedToken.selector, address(tokenA))); vm.startPrank(liquidityProvider); thunderLoan.redeem(tokenA, AMOUNT_LESS); vm.stopPrank(); } ``` ## Recommendations It would be suggested to add a check if that assetToken holds any balance of the ERC20, if so, then you cannot remove the mapping. ```diff function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) { if (allowed) { if (address(s_tokenToAssetToken[token]) != address(0)) { revert ThunderLoan__AlreadyAllowed(); } string memory name = string.concat("ThunderLoan ", IERC20Metadata(address(token)).name()); string memory symbol = string.concat("tl", IERC20Metadata(address(token)).symbol()); AssetToken assetToken = new AssetToken(address(this), token, name, symbol); s_tokenToAssetToken[token] = assetToken; emit AllowedTokenSet(token, assetToken, allowed); return assetToken; } else { AssetToken assetToken = s_tokenToAssetToken[token]; + uint256 hasTokenBalance = IERC20(token).balanceOf(address(assetToken)); + if (hasTokenBalance == 0) { delete s_tokenToAssetToken[token]; emit AllowedTokenSet(token, assetToken, allowed); + } return assetToken; } } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!