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

Disallowing a token may lead to the liquidity of said token's provided by LPs to be stuck in the contract

0xspryon

Summary

The funds of LPs for a token can be stuck in the contract if said token is disallowed while the LPs haven't yet redeemed all of the liquidity the provided for this token

Vulnerability Details

In src/protocol/ThunderLoan::setAllowedToken or src/protocol/ThunderLoanUpgraded::setAllowedToken, disallowing a token takes immediate effect regardless of whether there is still liquidity in the corresponding AssetToken contract that was provided prior to the disallow. This can lead to a permanent loss of funds of the LPs as they will unable to redeem said funds after the token has been disallowed.

POC

put below piece of code in `test/unit/ThunderLoanTest.t.sol` 
in the terminal, run `forge test --mt testPermanetLossOfFundsAfterDisallow -vvv`
function testPermanetLossOfFundsAfterDisallow() public setAllowedToken {
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.startPrank(liquidityProvider);
vm.expectRevert(abi.encodeWithSelector(ThunderLoan.ThunderLoan__NotAllowedToken.selector, address(tokenA)));
thunderLoan.redeem(tokenA, AMOUNT);
vm.stopPrank();
}

Impact

LP funds can become forever unredeemable.

Tools Used

Manual review

Recommendations

in src/protocol/ThunderLoan and src/upgradedProtocol/ThunderLoanUpgraded update the setAllowedToken method as shown below.

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];
+ require(assetToken.totalSupply() == 0, "Cannot disallow a token with a liquidity balance");
delete s_tokenToAssetToken[token];
emit AllowedTokenSet(token, assetToken, allowed);
return assetToken;
}
}

Note, we shouldn't verify if there's still liquidity in the token to be disallowed by checking the assetToken's balance of the token to be disabled.

require(token.balanceOf(address(assetToken)) == 0, "Cannot disallow ....");
this makes it vulnerable to an unexpected token balance attack if someone forcefully sends some liquidity to the assetToken address through routes other than the deposit method thereby, making it impossible to ever disallow this token as there isn't any means through which we can remove excess/undesired balance from the contract.

Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Admin Input/call validation
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

centralized owners can brick redemptions by unallowing a token

Support

FAQs

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