Summary
The thunderloanupgrade.sol storage layout is not compatible with the storage layout of thunderloan.sol which will cause storage collision and mismatch of variable to different data.
Vulnerability Details
Thunderloan.sol at slot 1,2 and 3 holds s_feePrecision, s_flashLoanFee and s_currentlyFlashLoaning, respectively, but the ThunderLoanUpgraded at slot 1 and 2 holds s_flashLoanFee, s_currentlyFlashLoaning respectively. the s_feePrecision from the thunderloan.sol was changed to a constant variable which will no longer be assessed from the state variable. This will cause the location at which the upgraded version will be pointing to for some significant state variables like s_flashLoanFee to be wrong because s_flashLoanFee is now pointing to the slot of the s_feePrecision in the thunderloan.sol and when this fee is used to compute the fee for flashloan it will return a fee amount greater than the intention of the developer.
s_currentlyFlashLoaning might not really be affected as it is back to default when a flashloan is completed but still to be noted that the value at that slot can be cleared to be on a safer side.
Impact
Fee is miscalculated for flashloan
users pay same amount of what they borrowed as fee
Tools Used
foundry
##POC
import {ThunderLoanUpgraded} from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol";
function upgradeThunderloan() internal {
thunderLoanUpgraded = new ThunderLoanUpgraded();
thunderLoan.upgradeTo(address(thunderLoanUpgraded));
thunderLoanUpgraded = ThunderLoanUpgraded(address(proxy));
}
function testSlotValuesBeforeAndAfterUpgrade() public setAllowedToken {
AssetToken asset = thunderLoan.getAssetFromToken(tokenA);
uint precision = thunderLoan.getFeePrecision();
uint fee = thunderLoan.getFee();
bool isflanshloaning = thunderLoan.isCurrentlyFlashLoaning(tokenA);
console.log("????SLOTS VALUE BEFORE UPGRADE????");
console.log("slot 0 for s_tokenToAssetToken =>", address(asset));
console.log("slot 1 for s_feePrecision =>", precision);
console.log("slot 2 for s_flashLoanFee =>", fee);
console.log("slot 3 for s_currentlyFlashLoaning =>", isflanshloaning);
upgradeThunderloan();
AssetToken assetUpgrade = thunderLoan.getAssetFromToken(tokenA);
uint feeUpgrade = thunderLoan.getFee();
bool isflanshloaningUpgrade = thunderLoan.isCurrentlyFlashLoaning(
tokenA
);
console.log("????SLOTS VALUE After UPGRADE????");
console.log("slot 0 for s_tokenToAssetToken =>", address(assetUpgrade));
console.log("slot 1 for s_flashLoanFee =>", feeUpgrade);
console.log(
"slot 2 for s_currentlyFlashLoaning =>",
isflanshloaningUpgrade
);
assertEq(address(asset), address(assetUpgrade));
assertEq(precision, feeUpgrade);
assertEq(isflanshloaning, isflanshloaningUpgrade);
}
Add the code above to thunderloantest.t.sol and run with forge test --mt testSlotValuesBeforeAndAfterUpgrade -vv
POC 2
function testFlashLoanAfterUpgrade() public setAllowedToken hasDeposits {
upgradeThunderloan();
uint256 amountToBorrow = AMOUNT * 10;
console.log("amount flashloaned", amountToBorrow);
uint256 calculatedFee = thunderLoan.getCalculatedFee(
tokenA,
amountToBorrow
);
AssetToken assetToken = thunderLoan.getAssetFromToken(tokenA);
vm.startPrank(user);
tokenA.mint(address(mockFlashLoanReceiver), amountToBorrow);
thunderLoan.flashloan(
address(mockFlashLoanReceiver),
tokenA,
amountToBorrow,
""
);
vm.stopPrank();
console.log("feepaid", calculatedFee);
assertEq(amountToBorrow, calculatedFee);
}
Add the code above to thunderloantest.t.sol and run forge test --mt testFlashLoanAfterUpgrade -vv to test for the second poc
Recommendations
The team should should make sure the the fee is pointing to the correct location as intended by the developer: a suggestion recommendation is for the team to get the feeValue from the previous implementation, clear the values that will not be needed again and after upgrade reset the fee back to its previous value from the implementation.
##POC for recommendation
function upgradeThunderloanFixed() internal {
thunderLoanUpgraded = new ThunderLoanUpgraded();
uint fee = thunderLoan.getFee();
thunderLoan.updateFlashLoanFee(0);
thunderLoan.upgradeTo(address(thunderLoanUpgraded));
thunderLoanUpgraded = ThunderLoanUpgraded(address(proxy));
thunderLoanUpgraded.updateFlashLoanFee(fee);
}
function testSlotValuesFixedfterUpgrade() public setAllowedToken {
AssetToken asset = thunderLoan.getAssetFromToken(tokenA);
uint precision = thunderLoan.getFeePrecision();
uint fee = thunderLoan.getFee();
bool isflanshloaning = thunderLoan.isCurrentlyFlashLoaning(tokenA);
console.log("????SLOTS VALUE BEFORE UPGRADE????");
console.log("slot 0 for s_tokenToAssetToken =>", address(asset));
console.log("slot 1 for s_feePrecision =>", precision);
console.log("slot 2 for s_flashLoanFee =>", fee);
console.log("slot 3 for s_currentlyFlashLoaning =>", isflanshloaning);
upgradeThunderloanFixed();
AssetToken assetUpgrade = thunderLoan.getAssetFromToken(tokenA);
uint feeUpgrade = thunderLoan.getFee();
bool isflanshloaningUpgrade = thunderLoan.isCurrentlyFlashLoaning(
tokenA
);
console.log("????SLOTS VALUE After UPGRADE????");
console.log("slot 0 for s_tokenToAssetToken =>", address(assetUpgrade));
console.log("slot 1 for s_flashLoanFee =>", feeUpgrade);
console.log(
"slot 2 for s_currentlyFlashLoaning =>",
isflanshloaningUpgrade
);
assertEq(address(asset), address(assetUpgrade));
assertEq(fee, feeUpgrade);
assertEq(isflanshloaning, isflanshloaningUpgrade);
}
Add the code above to thunderloantest.t.sol and run with forge test --mt testSlotValuesFixedfterUpgrade -vv.
it can also be tested with testFlashLoanAfterUpgrade function and see the fee properly calculated for flashloan
