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

Disallowing a token with active LP shares strands deposited funds

ye0lde

Root + Impact

Description

Liquidity providers deposit an allowed underlying token into ThunderLoan and receive shares in the corresponding AssetToken. Those shares are only redeemable through ThunderLoan.redeem(token, amount), which looks up the active AssetToken through s_tokenToAssetToken[token].

When the owner disallows a token, setAllowedToken(token, false) deletes s_tokenToAssetToken[token]. This makes existing LPs unable to redeem through the normal redemption path because redeem() first requires the token to be allowed. Reallowing the same token does not restore access to the old pool: it deploys a new empty AssetToken, while the old AssetToken still holds the deposited underlying and the LP still holds the old shares.

Source permalink: https://github.com/Cyfrin/2023-11-Thunder-Loan/blob/e8ce05f5530ca965165d41547b289604f873fdf6/src/protocol/ThunderLoan.sol#L161-L177

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);
}

Source permalink: https://github.com/Cyfrin/2023-11-Thunder-Loan/blob/e8ce05f5530ca965165d41547b289604f873fdf6/src/protocol/ThunderLoan.sol#L227-L243

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;
}
}

The same delete-and-recreate behavior is present in ThunderLoanUpgraded, so the PoC uses ThunderLoanUpgraded only to isolate this issue from the separate deposit-accounting bug in the original ThunderLoan.deposit().

Risk

Likelihood:

  • This occurs whenever the owner removes a token from the allowlist while existing LP shares or underlying remain in that token's AssetToken.

  • Normal token lifecycle operations can reach this state: a token is allowed, LPs deposit, the owner later removes the token because of an emergency, deprecation, migration, or configuration change.

Impact:

  • Existing LPs lose the protocol's normal redemption path for their shares. Their shares remain in the old AssetToken, but ThunderLoan.redeem() rejects the token once it is disallowed.

  • Reallowing the token does not restore access to the old reserves. The protocol creates a new empty AssetToken, leaving the old underlying stranded and unreachable through ThunderLoan.

Proof of Concept

Add the following test file:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { AssetToken } from "../src/protocol/AssetToken.sol";
import { BaseTest } from "./unit/BaseTest.t.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import { ThunderLoanUpgraded } from "../src/upgradedProtocol/ThunderLoanUpgraded.sol";
​
contract I04RedeemAfterTokenDisallowTest is BaseTest {
uint256 private constant DEPOSIT_AMOUNT = 1_000e18;
​
address private liquidityProvider = address(0xA11CE);
​
function test_I04_01_disallowingTokenStrandsExistingLpSharesAndUnderlying() public {
ThunderLoanUpgraded upgradedThunderLoan = _freshUpgradedThunderLoan();
AssetToken oldAssetToken = _allowAndDeposit(upgradedThunderLoan);
​
uint256 lpOldShares = oldAssetToken.balanceOf(liquidityProvider);
uint256 oldAssetUnderlying = tokenA.balanceOf(address(oldAssetToken));
​
upgradedThunderLoan.setAllowedToken(tokenA, false);
​
vm.expectRevert(abi.encodeWithSelector(ThunderLoanUpgraded.ThunderLoan__NotAllowedToken.selector, address(tokenA)));
vm.prank(liquidityProvider);
upgradedThunderLoan.redeem(tokenA, type(uint256).max);
​
AssetToken newAssetToken = upgradedThunderLoan.setAllowedToken(tokenA, true);
​
assertTrue(address(newAssetToken) != address(oldAssetToken), "reallow reused old asset token");
assertEq(tokenA.balanceOf(address(oldAssetToken)), oldAssetUnderlying, "old asset underlying not stranded");
assertEq(oldAssetToken.balanceOf(liquidityProvider), lpOldShares, "old shares not stranded");
assertEq(tokenA.balanceOf(address(newAssetToken)), 0, "new asset should have no underlying");
assertEq(newAssetToken.balanceOf(liquidityProvider), 0, "LP should have no new shares");
​
vm.prank(liquidityProvider);
upgradedThunderLoan.redeem(tokenA, type(uint256).max);
​
assertEq(tokenA.balanceOf(liquidityProvider), 0, "LP recovered underlying through new asset");
assertEq(tokenA.balanceOf(address(oldAssetToken)), oldAssetUnderlying, "old underlying was recovered");
assertEq(oldAssetToken.balanceOf(liquidityProvider), lpOldShares, "old shares were recovered");
}
​
function _freshUpgradedThunderLoan() private returns (ThunderLoanUpgraded upgradedThunderLoan) {
ThunderLoanUpgraded upgradedImplementation = new ThunderLoanUpgraded();
ERC1967Proxy upgradedProxy = new ERC1967Proxy(address(upgradedImplementation), "");
upgradedThunderLoan = ThunderLoanUpgraded(address(upgradedProxy));
upgradedThunderLoan.initialize(address(mockPoolFactory));
}
​
function _allowAndDeposit(ThunderLoanUpgraded upgradedThunderLoan) private returns (AssetToken assetToken) {
upgradedThunderLoan.setAllowedToken(tokenA, true);
assetToken = upgradedThunderLoan.getAssetFromToken(tokenA);
​
tokenA.mint(liquidityProvider, DEPOSIT_AMOUNT);
​
vm.startPrank(liquidityProvider);
tokenA.approve(address(upgradedThunderLoan), DEPOSIT_AMOUNT);
upgradedThunderLoan.deposit(tokenA, DEPOSIT_AMOUNT);
vm.stopPrank();
}
}

Run:

forge test --match-contract I04RedeemAfterTokenDisallowTest --match-test test_I04_01_disallowingTokenStrandsExistingLpSharesAndUnderlying -vvv

Result:

[PASS]
test_I04_01_disallowingTokenStrandsExistingLpSharesAndUnderlying()

The trace shows that the LP cannot redeem after the token is disallowed. After the owner reallows the same token, the new active AssetToken is different and empty, while the old AssetToken still holds 1000e18 underlying and the LP still holds 1000e18 old shares.

Recommended Mitigation

Do not delete or replace the AssetToken for a token while existing LP obligations remain. Separate the active product controls from the redemption accounting: disabling new deposits or flash loans should not remove the ability to redeem existing shares.

function setAllowedToken(IERC20 token, bool allowed) external onlyOwner returns (AssetToken) {
if (allowed) {
if (address(s_tokenToAssetToken[token]) != address(0)) {
- revert ThunderLoan__AlreadyAllowed();
+ s_tokenIsAllowed[token] = true;
+ emit AllowedTokenSet(token, s_tokenToAssetToken[token], allowed);
+ return s_tokenToAssetToken[token];
}
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;
} else {
AssetToken assetToken = s_tokenToAssetToken[token];
- delete s_tokenToAssetToken[token];
+ s_tokenIsAllowed[token] = false;
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
}
}

Then gate new deposits and flash loans on the allowlist flag, while allowing redeem() to use the existing s_tokenToAssetToken[token] whenever an AssetToken exists.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day 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!