Summary
The ThunderLoan contract has variable stored in different storage spots compared to ThunderLoanUpgraded contract. During upgrading of the contract, the delegate calls will be pointed to the wrong storage spot.
Vulnerability Details
ThunderLoan has the following storage variables:
uint256 private s_feePrecision;
uint256 private s_flashLoanFee;
ThunderLoanUpgraded has the following storage variables:
uint256 private s_flashLoanFee;
After upgrading the contract, calling getFee() and updateFlashLoanFee(uint256 newFee) will be calling/updating storage slot 0 of the original contract. Meaning getFee() will be getting the s_feePrecision and updateFlashLoanFee() will be updating s_feePrecision with the new fee because in ThunderLoanUpgraded, s_flashLoanFee which belongs to storage 0.
Impact
This can potentially cause the fee from the flashloan to be an unexpected value, which is a risk to either the users or the liquidity providers.
Tools Used
Foundry
Create the following:
Sample script to upgrade thunderLoan.sol (have to install devopstools from chainaccel):
import { Script } from "forge-std/Script.sol";
import { DevOpsTools } from "lib/foundry-devops/src/DevOpsTools.sol";
import { ThunderLoanUpgraded } from "../src/upgradedProtocol/ThunderLoanUpgraded.sol";
import { ThunderLoan } from "../src/protocol/ThunderLoan.sol";
contract UpgradeThunderLoan is Script {
function run() external returns (address) {
address mostRecentlyDeployed = DevOpsTools.get_most_recent_deployment("ERC1967Proxy", block.chainid);
vm.startBroadcast();
ThunderLoanUpgraded thunderLoanUpgrade = new ThunderLoanUpgraded();
vm.stopBroadcast();
address proxy = upgradedThunderLoan(mostRecentlyDeployed, address(thunderLoanUpgrade));
return proxy;
}
function upgradedThunderLoan(address proxyAddress, address newThunderLoan) public returns (address) {
vm.startBroadcast();
ThunderLoan proxy = ThunderLoan(proxyAddress);
proxy.upgradeTo(address(newThunderLoan));
vm.stopBroadcast();
return address(proxy);
}
}
Sample script to deploy & test the upgrade:
import { console, Test } from "forge-std/Test.sol";
import { DeployThunderLoan } from "../../script/DeployThunderLoan.s.sol";
import { UpgradeThunderLoan } from "../../script/UpgradeThunderLoan.s.sol";
import { ThunderLoan } from "../../src/protocol/ThunderLoan.sol";
import { ThunderLoanUpgraded } from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol";
contract DeployAndTestUpgrade is Test {
DeployThunderLoan public deployer;
UpgradeThunderLoan public upgrader;
address public OWNER = makeAddr("owner");
function setUp() public {
deployer = new DeployThunderLoan();
upgrader = new UpgradeThunderLoan();
}
function testDeploy() public {
address proxyAddress = deployer.deployThunderLoan();
uint256 expectedValue = 3e15;
assertEq(ThunderLoan(proxyAddress).getFee(), expectedValue);
}
function testUpgrade() public {
address proxyAddress = deployer.deployThunderLoan();
ThunderLoanUpgraded thunderLoanUpgraded = new ThunderLoanUpgraded();
vm.startPrank(ThunderLoan(proxyAddress).owner());
ThunderLoan(proxyAddress).transferOwnership(msg.sender);
vm.stopPrank();
address proxy = upgrader.upgradedThunderLoan(proxyAddress, address(thunderLoanUpgraded));
uint256 expectedValue = 3e15;
assertEq(expectedValue, ThunderLoanUpgraded(proxy).getFee());
}
}
Run the test:
forge test --mt testDeploy() -vvvv
We can see that before upgrading the contract, the expected value for the Fee is 3e15:
├─ [764] ERC1967Proxy::getFee() [staticcall]
│ ├─ [369] ThunderLoan::getFee() [delegatecall]
│ │ └─ ← 3000000000000000 [3e15]
│ └─ ← 3000000000000000 [3e15]
└─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.21ms
forge test --mt testUpgrade() -vvvv
After upgrading, getFee returns 1e18 and the assertion will fail like below:
├─ [742] ERC1967Proxy::getFee() [staticcall]
│ ├─ [347] ThunderLoanUpgraded::getFee() [delegatecall]
│ │ └─ ← 1000000000000000000 [1e18]
│ └─ ← 1000000000000000000 [1e18]
├─ emit log(: Error: a == b not satisfied [uint])
├─ emit log_named_uint(key: Left, val: 3000000000000000 [3e15])
├─ emit log_named_uint(key: Right, val: 1000000000000000000 [1e18])
├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001)
│ └─ ← ()
└─ ← ()
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.22ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)
Recommendations
Keep storage spots the same like below for the upgrade & do not remove them unnecessarily.
uint256 private s_feePrecision;
uint256 private s_flashLoanFee;
Also FEE_PRECISION is a constant and does not take up a storage space and should be put back in place like the above. (I hope I am correct here),
uint256 public constant FEE_PRECISION = 1e18;
