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

`setAllowedToken(token, false)` orphans the AssetToken holding every liquidity provider's deposit, and no function in either implementation can reach those funds again

augustwh11

De-listing a token deletes the pointer to the contract that custodies the deposits, and re-listing deploys a different one

Description

  • Underlying deposits do not live in ThunderLoan; each token's balance sits in its own AssetToken, and s_tokenToAssetToken is the only record of which AssetToken belongs to which token.

  • setAllowedToken(token, false) deletes that entry. It does not migrate, close or drain anything — the AssetToken keeps custody of every LP's underlying, and the only handle to it is gone.

// src/protocol/ThunderLoan.sol:227-244
function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) {
if (allowed) {
if (address(s_tokenToAssetToken[token]) != address(0)) {
revert ThunderLoan__AlreadyAllowed();
}
...
@> AssetToken assetToken = new AssetToken(address(this), token, name, symbol); // @> always a NEW one
s_tokenToAssetToken[token] = assetToken;
...
} else {
AssetToken assetToken = s_tokenToAssetToken[token];
@> delete s_tokenToAssetToken[token]; // @> the only pointer to the funds is dropped
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
}
}

There is no way back to the money. AssetToken::transferUnderlyingTo is onlyThunderLoan, and the sole call site inside ThunderLoan is redeem, which is gated on the mapping being populated:

// src/protocol/AssetToken.sol:76-78
function transferUnderlyingTo(address to, uint256 amount) external onlyThunderLoan {
i_underlying.safeTransfer(to, amount);
}
// src/protocol/ThunderLoan.sol:161-178
function redeem(IERC20 token, uint256 amountOfAssetToken)
external
revertIfZero(amountOfAssetToken)
@> revertIfNotAllowedToken(token) // @> reverts the moment the token is de-listed
{
@> AssetToken assetToken = s_tokenToAssetToken[token]; // @> and afterwards resolves to a DIFFERENT contract

Re-listing is not a remedy — it deepens the problem. The allowed branch has no lookup for a pre-existing AssetToken; it unconditionally deploys a new one with a fresh supply and a reset exchange rate. The LPs' shares are in the old contract, the accounting is in the new one, and the two are never reconnected.

Recoverability, stated accurately. The funds are not mathematically lost. transferUnderlyingTo is gated on i_thunderLoan, which is the proxy address, and that does not change across an upgrade — so the owner can deploy a new implementation that restores the mapping entry and recover everything. I verified this works. What the finding claims is narrower and still true: no function in either in-scope implementation can reach those funds, so recovery requires writing and shipping new code, under exactly the same trusted role that stranded them, and only if the mistake is noticed at all.

There is also a nasty second-order effect once the token has been re-listed. redeem(token, type(uint256).max) resolves to the caller's balance in the new AssetToken, which is zero — and revertIfZero guards the raw parameter, not the resolved amount. So the LP's "withdraw everything" call succeeds, emits Redeemed, and transfers nothing:

// src/protocol/ThunderLoan.sol:171-177
if (amountOfAssetToken == type(uint256).max) {
@> amountOfAssetToken = assetToken.balanceOf(msg.sender); // @> 0 in the new AssetToken
}
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); // @> transfers 0, returns success

Risk

Likelihood:

  • Low — the call is onlyOwner. I am not claiming a malicious owner; that would be a trust assumption, not a bug.

  • What makes it more than theoretical is that setAllowedToken(token, false) reads like an ordinary, reversible administrative action. Nothing in the function name, the signature, the events or a NatSpec comment (there is none) suggests it destroys custody. An owner de-listing a token to pause it during an incident, or to clean up an unused market, would have no reason to expect this. The allowed flag being a boolean parameter on a single function actively suggests the operation is symmetric.

  • There is no timelock, no guard on a non-zero balance, and no test in the repository that de-lists a token that has deposits in it.

Impact:

  • Total loss of every liquidity provider's principal for that token as far as the shipped code is concerned. No function in either in-scope implementation can move those funds again; only a newly written and deployed implementation can, which puts recovery outside the audited system and behind the same privileged role that caused it.

  • The LPs receive no signal. Their AssetToken balance is untouched and their share price is unchanged; only the withdrawal fails. After a re-listing it does not even fail — it silently returns zero, which is materially worse than a revert, because a wallet or integrator sees a confirmed Redeemed event.

Impact High × Likelihood Low ⇒ Medium.

Proof of Concept

Save as test/PoC_M03.t.sol and run forge test --mt test_M03 -vv:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { Test, console2 } from "forge-std/Test.sol";
import { ThunderLoan } from "../src/protocol/ThunderLoan.sol";
import { AssetToken } from "../src/protocol/AssetToken.sol";
import { ERC20Mock } from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import { MockPoolFactory } from "./mocks/MockPoolFactory.sol";
​
contract PoC_M03 is Test {
ThunderLoan thunderLoan;
ERC20Mock tokenA;
address liquidityProvider = makeAddr("liquidityProvider");
uint256 constant LP_DEPOSIT = 1000e18;
​
function setUp() public {
MockPoolFactory factory = new MockPoolFactory();
tokenA = new ERC20Mock();
factory.createPool(address(tokenA));
thunderLoan = ThunderLoan(address(new ERC1967Proxy(address(new ThunderLoan()), "")));
thunderLoan.initialize(address(factory));
thunderLoan.setAllowedToken(tokenA, true);
​
tokenA.mint(liquidityProvider, LP_DEPOSIT);
vm.startPrank(liquidityProvider);
tokenA.approve(address(thunderLoan), LP_DEPOSIT);
thunderLoan.deposit(tokenA, LP_DEPOSIT);
vm.stopPrank();
}
​
function test_M03_setAllowedTokenFalseStrandsLpFunds() public {
AssetToken oldAsset = thunderLoan.getAssetFromToken(tokenA);
uint256 stranded = tokenA.balanceOf(address(oldAsset));
​
// the owner de-lists the token - a routine-looking administrative action
thunderLoan.setAllowedToken(tokenA, false);
​
vm.prank(liquidityProvider);
vm.expectRevert(); // ThunderLoan__NotAllowedToken
thunderLoan.redeem(tokenA, type(uint256).max);
​
// re-listing does not restore access: a brand new AssetToken is deployed
thunderLoan.setAllowedToken(tokenA, true);
AssetToken newAsset = thunderLoan.getAssetFromToken(tokenA);
​
console2.log("underlying stranded in old AssetToken :", stranded);
console2.log("LP shares in the old AssetToken :", oldAsset.balanceOf(liquidityProvider));
console2.log("LP shares in the new AssetToken :", newAsset.balanceOf(liquidityProvider));
console2.log("underlying in the new AssetToken :", tokenA.balanceOf(address(newAsset)));
​
assertTrue(address(newAsset) != address(oldAsset), "a different AssetToken");
assertEq(newAsset.balanceOf(liquidityProvider), 0, "the LP's shares are in the orphan");
assertEq(tokenA.balanceOf(address(oldAsset)), stranded, "and so is the underlying");
​
// worse than a revert: "withdraw everything" now succeeds and pays nothing
uint256 walletBefore = tokenA.balanceOf(liquidityProvider);
vm.prank(liquidityProvider);
thunderLoan.redeem(tokenA, type(uint256).max);
console2.log("redeem(max) after re-listing pays out :", tokenA.balanceOf(liquidityProvider) - walletBefore);
assertEq(tokenA.balanceOf(liquidityProvider), walletBefore, "redeem succeeds and transfers zero");
}
}

Output:

[PASS] test_M03_setAllowedTokenFalseStrandsLpFunds() (gas: 1961996)
Logs:
underlying stranded in old AssetToken : 1000000000000000000000
LP shares in the old AssetToken : 1000000000000000000000
LP shares in the new AssetToken : 0
underlying in the new AssetToken : 0
redeem(max) after re-listing pays out : 0

Recommended Mitigation

De-listing should stop new exposure, not custody. Keep the AssetToken reachable and refuse to abandon a funded one.

+ mapping(IERC20 => AssetToken) private s_retiredAssetToken;
+ error ThunderLoan__AssetTokenNotEmpty(uint256 remaining);
+
function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) {
if (allowed) {
if (address(s_tokenToAssetToken[token]) != address(0)) {
revert ThunderLoan__AlreadyAllowed();
}
+ // re-listing must resurrect the original AssetToken, never mint a second one
+ AssetToken retired = s_retiredAssetToken[token];
+ if (address(retired) != address(0)) {
+ s_tokenToAssetToken[token] = retired;
+ delete s_retiredAssetToken[token];
+ emit AllowedTokenSet(token, retired, allowed);
+ return retired;
+ }
string memory name = string.concat("ThunderLoan ", IERC20Metadata(address(token)).name());
...
} else {
AssetToken assetToken = s_tokenToAssetToken[token];
+ uint256 remaining = token.balanceOf(address(assetToken));
+ if (remaining != 0) {
+ revert ThunderLoan__AssetTokenNotEmpty(remaining);
+ }
+ s_retiredAssetToken[token] = assetToken;
delete s_tokenToAssetToken[token];

If de-listing a funded market has to remain possible, then it must not also disable withdrawals: drop revertIfNotAllowedToken from redeem and resolve the AssetToken from a mapping that de-listing never clears, so LPs can always exit a market that has been closed to new deposits.

Separately, and independent of the fix above, redeem should not treat a resolved amount of zero as success:

if (amountOfAssetToken == type(uint256).max) {
amountOfAssetToken = assetToken.balanceOf(msg.sender);
+ if (amountOfAssetToken == 0) {
+ revert ThunderLoan__CantBeZero();
+ }
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 12 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!