function changeController() has rug potential as admin can unilaterally withdraw all user funds from both risk and insure vaults

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L295 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L360-L366

Vulnerability details

Impact

Admin can rug all user funds in every vaults deployed by changing the controller address.

Proof of Concept

Controller can be changed by admin anytime without any warning to users.

Vault.sol#L295

    function changeController(address _controller) public onlyFactory {
        if(_controller == address(0))
            revert AddressZero();
        controller = _controller;
    }

Tokens in the vaults can then be called by the malicious contract to be transferred to their own address with sendTokens().

Vault.sol#L360-L366

    function sendTokens(uint256 id, address _counterparty)
        public
        onlyController
        marketExists(id)
    {
        asset.transfer(_counterparty, idFinalTVL[id]);
    }

Recommended Mitigation Steps

Allow the change of controller address only in the VaultFactory(). This way, markets that have been created cannot have a different controller address, so users can be made aware of the change before choosing to make deposit of assets.

[MiguelBits]

implemented timelock as issued in another finding

[HickupHH3]

admin privilege finding, rationale for QA explained here

[HickupHH3]

user's primary QA

[yixxas]

Hi @HickupHH3. Would like to seek clarifications on why this was downgraded to QA. In the rationale given, "Those that explained the impact and vulnerability in detail will be grouped together with medium severity ".

I believe the way a compromised admin can rug is clear in this report. changeController() can be used to change controller to any address. This address can then be used to call sendTokens() to steal all assets in every vault.

[HickupHH3]

Took another look at this.

I disagree with the recommended fix. The purpose for having the controller changeable in the deployed instances is well intentioned for upgradeability purposes: perhaps new features are to be added to the controller, and migration to a new controller for all existing vaults is required to incur less technical debt. Needing to maintain legacy controllers isn't great from a devops POV.

That said, based on my rationale, the issue should stand as a medium severity issue until we have the introduction of centralisation reports. Kenzo said it well:

IMO most of these trusted-actor issues basically just describe general properties of the crypto/governance ecosystems, and do not reflect a novel problem in the design/implementation (which wardens are paid to discover). Because of this, and because of the circular logic, I believe we should change the rules and add a dedicated centralization report.