Part 2

Summary

The BaseAdapter::__BaseAdapter_init function uses initializer modifier instead of onlyInitializing. This allows anyone to call the function and the child contracts can not initialized because they call __BaseAdapter_init function that can be executed only once.

Vulnerability Details

The contract BaseAdapter is UUPSUpgradeable and it is inherited by 3 child contracts: CurveAdapter, UniswapV2Adapter and UniswapV3Adapter. The BaseAdapter contract has __BaseAdapter_init function that is called in the initializer functions of the child contracts. The problem is that the parent contract doesn't have constructor that call _disableInitializers function and __BaseAdapter_init function has initializer modifier.

The consequences of this are two. The first one is that anyone can call the __BaseAdapter_init function and set owner address. This means that the malicious owner can call owner's functions in BaseAdapter contract (setSwapConfig, setSlippageTolerance, setDeadline, and upgrade contract).

The following test shows how a malicious user can call the BaseAdapter::__BaseAdapter_init function, sets his address as owner and then can access all functions from the contract that has onlyOwner modifier. This allows him to change sensitive variables for the protocol:

For simplicity I removed the body of the setSwapAssetConfig function and added only a console.log:

The result from this test is the following:

If the __BaseAdapter_init function uses onlyInitializing modifier instead of initializer, the test will fail, the function will be protected and only callable from the initializers functions of the child contracts:

The second problem is that the child contracts CurveAdapter, UniswapV2Adapter and UniswapV3Adapter can not be initialized properly.
The initializer modifier ensures that the function can be called only once during initialization. This means when one of the child contracts calls BaseAdapter::__BaseAdapter_init function, the others child contracts will be unable to call this function again and respectively they can not be initialized. According to the Openzeppelin documentation the onlyInitializing modifier should be used in the parent contract to allow correctly initialization of the child contracts.

Impact

The BaseAdapter::__BaseAdapter_init function uses initializer modifier instead of onlyInitializing. This means, anyone can call the BaseAdapter::__BaseAdapter_init function and set his address as owner and gain access to all crucial functions that has onlyOwner modifier. Also, the child contracts of the BaseAdapter are not able to be initialized, because their intialize functions call the __BaseAdapter_init function, but the initializer modifier allows only one call to the function.

Tools Used

Manual Review, Foundry

Recommendations

Add a constructor that calls _disableInitializers to the BaseAdapter function and use onlyInitializing function instead of initializer in __BaseAdapter_init function.