ast3ros - Vault Factory ownership can be changed immediately and bypass timelock delay

[sherlock-admin]

ast3ros

medium

Vault Factory ownership can be changed immediately and bypass timelock delay

Summary

The VaultFactoryV2 contract is supposed to use a timelock contract with a delay period when changing its owner. However, there is a loophole that allows the owner to change the owner address instantly, without waiting for the delay period to expire. This defeats the purpose of the timelock contract and exposes the VaultFactoryV2 contract to potential abuse.

Vulnerability Detail

In project description, timelock is required when making critical changes. Admin can only configure new markets and epochs on those markets.

    2) Admin can configure new markets and epochs on those markets, Timelock can make cirital changes like changing the oracle or whitelisitng controllers.

The VaultFactoryV2 contract has a changeOwner function that is supposed to be called only by the timelock contract with a delay period.

function changeOwner(address _owner) public onlyTimeLocker {
        if (_owner == address(0)) revert AddressZero();
        _transferOwnership(_owner);
    }

The VaultFactoryV2 contract inherits from the Openzeppelin Ownable contract, which has a transferOwnership function that allows the owner to change the owner address immediately. However, the transferOwnership function is not overridden by the changeOwner function, which creates a conflict and a vulnerability. The owner can bypass the timelock delay and use the transferOwnership function to change the owner address instantly.

    function transferOwnership(address newOwner) public virtual onlyOwner {
        require(newOwner != address(0), "Ownable: new owner is the zero address");
        _transferOwnership(newOwner);
    }

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

Impact

The transferOwnership is not worked as design (using timelock), the timelock delay become useless. This means that if the owner address is hacked or corrupted, the attacker can take over the contract immediately, leaving no time for the protocol and the users to respond or intervene.

Code Snippet

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

Tool used

Manual Review

Recommendation

Override the transferOwnership function and add modifier onlyTimeLocker.

[thangtranth]

Escalate for 10 USDC.

This issue is different from #501 and cannot be ignored. It is not related to using two steps to change ownership. The problem here is that the transferOwnership function in the Ownable contract is not overridden as it should be. This allows the owner to change the ownership without going through the timelock. This creates a severe security risk and undermines the trust and transparency of the protocol as stated in spec.

[sherlock-admin]

Escalate for 10 USDC.

This issue is different from #501 and cannot be ignored. It is not related to using two steps to change ownership. The problem here is that the transferOwnership function in the Ownable contract is not overridden as it should be. This allows the owner to change the ownership without going through the timelock. This creates a severe security risk and undermines the trust and transparency of the protocol as stated in spec.

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

Not a duplicate of #501 and can be considered a valid medium since this identifies the issue that transferOwnership is not overridden and needs to have `onlyTimeLocker' modifier,

[sherlock-admin]

Escalation accepted

Not a duplicate of #501 and can be considered a valid medium since this identifies the issue that transferOwnership is not overridden and needs to have `onlyTimeLocker' modifier,

This issue's escalations have been accepted!

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

[hrishibhat]

Lead Judge comment:

looks valid, maybe med, if they intend to do it without a delay is one thing and to be documented, but if a function just left not overriden it's a bug

Sponsor comment:

Actually thats valid issue, .... fixing this will make this action more complicated. My thinking is to add a direct function on timelocker which lets timelocker execute the owner (deployer) change without 7day queue.

[3xHarry]

FIX RP: https://github.com/Y2K-Finance/Earthquake/pull/147 - last two commits

[IAm0x52]

Fix looks good. changeOwner has been removed and transferOwnership has been overridden to allow only timelocker