0xnirlin - Can change controller on factory without checking if the factory on controller is set to right factory.

[sherlock-admin]

0xnirlin

medium

Can change controller on factory without checking if the factory on controller is set to right factory.

Summary

Can change controller on factory without checking if the factory on controller is set to right factory.

Vulnerability Detail

We can change controller on factory by following code

    function changeController(
        uint256 _marketId,
        address _controller
    ) public onlyTimeLocker controllerIsWhitelisted(_controller) {

        // @audit-issue whitelist check is placed but there is not check that if on controller the factory is set to this one
        if (_controller == address(0)) revert AddressZero();

        address[2] memory vaults = marketIdToVaults[_marketId];

        if (vaults[0] == address(0) || vaults[1] == address(0)) {
            revert MarketDoesNotExist(_marketId);
        }

        IVaultV2(vaults[0]).changeController(_controller);
        IVaultV2(vaults[1]).changeController(_controller);

        emit ControllerChanged(_marketId, _controller, vaults[0], vaults[1]);
    }

So controller will be set without checking with the controller itself, just checking for whitelist

Impact

Unexpected reverts and behaviour will occur.

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultFactoryV2.sol#L286

Tool used

Manual Review

Recommendation

Add necessary checks on both side for the compatibility

[pauliax]

Escalate for 10 USDC.

Clearly, it is not a duplicate of #110. This issue should be invalid altogether, the check is not necessary because only the factory can change the controller: https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L247

[sherlock-admin]

Escalate for 10 USDC.

Clearly, it is not a duplicate of #110. This issue should be invalid altogether, the check is not necessary because only the factory can change the controller: https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L247

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xnirlin]

Ok agreed, not a duplicate of this one but it doesn't mean it is invalid.

If we look in controller contract, factory is set via constructor and is immutable variable and there is no setter function for factory as it is immutable.

We can set controller in the factory contract for the particular factory, problem is while setting controller we never check if the particular controller has factory assigned as this current address.

Same goes for changeController function, it never checks either whether the contract we are setting has the factory address of this or not.

From design of Y2k if a wrong factory address is set on the controller, it will lead to never triggering the depeg , endEpoch and nullEpoch functions.

Though it requires the operator mistake, but impact of the mistake goes far beyond, so marked it as medium.

Solution could be either make factory mutable and add a setter function or develop some mechanism to cross check.

[0xnirlin]

Escalate for 10 USDC

Issue is not duplicate but a valid one as described above

[sherlock-admin]

Escalate for 10 USDC

Issue is not duplicate but a valid one as described above

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Invalid issue Not a duplicate of #110 The roles are trusted to make these necessary changes.

[sherlock-admin]

Escalation accepted

Invalid issue The roles are trusted to make these necessary changes.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.