Root + Impact

• Root: There's no access control on the ThunderLoan::initialize function, and we know initialize is a critical function.

• Impact: The actual owner will hardly have any control if someone else calls the initialize function other than him.

Description

• Normal Behaviour: In the UUPS pattern, as the implementation contract is deployed, its initialize function has to be called as early as possible to avoid any annoying scenarios. And that function is obviously called by the owner itself, not by someone else.

• Issue:

  • The initialize function lacks any type of access control. This means, after the contract is deployed, there will be a race condition to call the initialize function. And whoever calls it might be having a certain amount of control over this contract itself.

  • Even if the actual owner tries to call the initialize function immediately after deploying the contract or through some script. Any malicious user or miner can easily front-run his transaction by either paying high fees or using some other means.

Risk

• Likelihood: Medium

  • The attacker definitely needs to force his transaction ahead of the owner. So this will need some funds, constant monitoring of the chain, and the right timing.

• Impact: High

  • Many of the owner-related important functions can fall under the attacker's control.

  • The control is useless now and has to be flagged by the protocol to be not used. But how many, and for how long?

  • Damages the trust and reputation of the protocol

Proof of Concept

1. In this PoC, we will just demonstrate how anyone can call this initialize function, assuming that front-running is taking place.

2. Include this test in the test/unit/BaseTest.t.sol

   function test__MaliciousUserInitializedTheContract() public {

   // Let's create a new thunderLoan contract

   ThunderLoan naiveThunderLoan = new ThunderLoan();

   ​

   // It will need a new proxy as well

   ERC1967Proxy naiveProxy = new ERC1967Proxy(address(naiveThunderLoan), "");

   naiveThunderLoan = ThunderLoan(address(naiveProxy));

   ​

   // Malicious user

   address maliciousUser = address(123);

   // Here, the malicious user front runs the initialize function call

   vm.prank(maliciousUser);

   naiveThunderLoan.initialize(address(mockPoolFactory));

   ​

   // Actual owner's transaction is reverted

   vm.expectRevert();

   naiveThunderLoan.initialize(address(mockPoolFactory));

   }

3. Run it using:

   forge test --mt test__MaliciousUserInitializedTheContract

Recommended Mitigation

1. Either the contract should create a pre-discussed admin address in the contract itself, and thus it will be re-checked whether that admin is the caller of initialize function

   + address constant ADMIN = address("our_admin_address");

   ...

   ​

   function initialize(address tswapAddress) external initializer {

   + if (msg.sender != ADMIN) {

   + revert();

   + }

   ​

   // ...

   }

2. Or, use some kind of access control like onlyOwner, which just lets the deployer of the contract call this initialize function.