Root + Impact

• Root: The upgraded ThunderLoanUpgraded contract is using initializer modifier in its ThunderLoanUpgraded::initialize function.

• Impact: This leads to ThunderLoanUpgraded not being able to call initialize at all, as it is been initialized again with the same version.

Description

• We know that this protocol is thinking about upgrading the implementation contract at some point, and that's why ThunderLoanUpgraded was under scope. Also, it uses UUPS and Initializable for those upgrades and initializing the stuff.

• It's advised to initialize as early as possible, and the initialize function is meant to have initializer modifier, which restricts any further initialization and sets the version number to 1 as well. That means, every time the logic is updated (i.e. updating the implementation contract), we will get a new version.

• But, in the ThunderLoanUpgraded contract — which happens to be the next implementation — we are using that initializer modifier again in the initialize function. And this will lead to the same version number, i.e. 1 and thus, reverting it right away.

• The protocol completely missed the basic that new implementations are meant to use reinitializer with some version number.

Risk

• Likelihood: High

  • This system is expecting the upgrade to ThunderLoanUpgraded

  • Usually, the initialize function is always called after making the upgrade

• Impact: Medium

  • Not able to call initialize function at all

  • The new upgrade will be halted

  • Various hidden issues probably

Proof Of Concept

1. Add the following test to test/unit/ThunderLoanTest.t.sol

   function test__ReinitializationFailsInThunderLoanUpgraded() public {

   // Let's upgrade the implementation contract

   ThunderLoanUpgraded thunderLoanUpgraded = new ThunderLoanUpgraded();

   UUPSUpgradeable(thunderLoan).upgradeToAndCall(address(thunderLoanUpgraded), "");

   ​

   // Let's try to reinitialize the contract

   thunderLoanUpgraded = ThunderLoanUpgraded(address(proxy));

   vm.expectRevert();

   thunderLoanUpgraded.initialize(address(mockPoolFactory));

   }
   
   

2. Run the test using:

   forge test --mt test__ReinitializationFailsInThunderLoanUpgraded

Recommended Mitigation

The initializer modifier should be replaced by reinitializer(2) if the version number is 2.