c7e7eff - Custom factory can be used to create valid looking Diversifiers to steal other users funds

[sherlock-admin]

c7e7eff

high

Custom factory can be used to create valid looking Diversifiers to steal other users funds

c7e7eff High

Summary

The UniV3OracleImpl's' initializer function can be called multiple times. Although this function can only be called by the deployer, it allows to override important parameters like owner, $defaultScaledOfferFactor, $defautFee and defaultPeriod. When a Diversifier is created for use in a trustless environment (e.g. people preferring different tokens like one person USDC while another might want EUROC) and the ownership of the Diversifier/PassThroughWallet contract is either renounced or behind a multi-sig as is recommended in the docs, the deployer of the oracle contract can still change these parameters. Although the canonical UniV3OracleFactory contract cannot call the initializer of an existing UniV3OracleImpl contract a malicious user can create or use a factory which does allow this (even simply an upgradable proxy in front of the canonical Factory would suffice for this as it would allow the malicious user to upgrade to a contract that would allow this). The malicious user can then share the created Diversifier with other users and renounce the ownership of the Diversifier contract. As the only checks included in the documentation for trustlessness is to check the ownership of the Diversifier (and underlying contracts) they will see that the ownership is renounced and trust the contract. However as the Oracle still has the malicious (or upgradeable) factory designated as uniV3OracleFactory the oracle is fully manipulatable by the malicious user who can now steal the other user's funds (for instance by setting a very low or 0 ScaledOfferFactor and calling flash as a trader thereby getting all the funds out of the sapper contract an giving nothing to the beneficiary).

Vulnerability Detail

The initializer of the UniswapV3OracleImplcontract has no modifier to prevent it being called multiple times and simply checks that the msg.sender corresponds with its deployer and then sets important variables.

    function initializer(InitParams calldata params_) external {
        if (msg.sender != uniV3OracleFactory) revert Unauthorized();
        __initOwnable(params_.owner);
        $paused = params_.paused;
        $defaultFee = params_.defaultFee;
        $defaultPeriod = params_.defaultPeriod;
        $defaultScaledOfferFactor = params_.defaultScaledOfferFactor;

The SwapperFactory contract taking the user provided CreateSwapperParams data to call the _parseIntoOracle function and use the returned oracle address for the swapper contracts

    function createSwapper(CreateSwapperParams calldata params_) external returns (SwapperImpl swapper) {
        OracleImpl oracle = params_.oracleParams._parseIntoOracle();
        swapper = SwapperImpl(payable(address(swapperImpl).clone()));
        SwapperImpl.InitParams memory swapperInitParams = SwapperImpl.InitParams({
            owner: params_.owner,
            paused: params_.paused,
            beneficiary: params_.beneficiary,
            tokenToBeneficiary: params_.tokenToBeneficiary,
            oracle: oracle
        });

The _parseIntoOracle function in OracleParams using (malicious) user provided factory address to create the oracle:

function _parseIntoOracle(OracleParams calldata oracleParams_) returns (OracleImpl) {
    CreateOracleParams calldata createOracleParams = oracleParams_.createOracleParams;
    return createOracleParams.factory.createOracle(createOracleParams.data);

Important to note is that the clone that is created here can point to the canonical implementation used by the rest of the system. So in this case a well-intentioned user can be fooled to trust the implementation contract.

Impact

In a trust-less environment a malicious user can create a Diversifier and renounce the ownership but still be able to regain ownership of the underlying Oracle contract to steal users funds.

Note that this kind of attack is also possible for the PassThroughWallerImpl and SwapperImpl contracts as they too have initializers that can be called by their deployers and only check the msg.sender to be their deployer contract. Although it would probably be more noticeable on the PassThroughWallerImpl as the creation of the Diversifier would be done on a Factory that is not the canonical one which might draw attention when looking at etherscan to a more sophisticated user.

Code Snippet

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-oracle/src/UniV3OracleImpl.sol#L123-L125

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-oracle/src/UniV3OracleFactory.sol#L45-L47

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-oracle/src/peripherals/OracleParams.sol#L28-L29

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-swapper/src/SwapperFactory.sol#L40-L50

Tool used

Manual Review

Recommendation

Multiple actions are probably necessary to prevent this kind of attack. Firstly the documentation should clearly state what less sophisticated users should check for trustless environments. Also make sure the initalizer function can only be called once.

Also consider blocking any calls from clones deployed by non canonical factories in the implementation contracts. This would prevent malicious users to use the canonical implementation contract in order to make their maliciously created clones look like valid clones (i.e. by looking at etherscan and seeing that the team deployed verified contracts are used by the Diversifier)

An additional step could be to add a functionality to the main Diversifier factory or a separate Registry contract to allow non-sophisticated users to check which underlying contracts of a Diversifier are actually the canonical ones and which ones are not. This would obviously need more coordination between the underlying contracts to register each canonically deployed clone (which is already partially done in the SwapperFactory contract.)