Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

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

Open
sherlock-admin opened this issue Mar 27, 2023 · 7 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 27, 2023

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.

@github-actions github-actions bot closed this as completed Apr 3, 2023
@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 3, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue labels Apr 11, 2023
@thangtranth
Copy link

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
Copy link
Contributor Author

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Apr 12, 2023
@hrishibhat
Copy link

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
Copy link
Contributor Author

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.

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels Apr 27, 2023
@hrishibhat hrishibhat reopened this Apr 27, 2023
@hrishibhat hrishibhat added Medium A valid Medium severity issue Sponsor Confirmed The sponsor acknowledged this issue is valid and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Non-Reward This issue will not receive a payout labels Apr 27, 2023
@3xHarry 3xHarry added the Will Fix The sponsor confirmed this issue will be fixed label Apr 28, 2023
@hrishibhat
Copy link

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.

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Apr 28, 2023
@3xHarry
Copy link

3xHarry commented May 1, 2023

FIX RP: Y2K-Finance/Earthquake#147 - last two commits

@IAm0x52
Copy link
Collaborator

IAm0x52 commented May 5, 2023

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

3xHarry added a commit to Y2K-Finance/Earthquake that referenced this issue May 8, 2023
@jacksanford1 jacksanford1 changed the title ast3ros - [M-2] Vault Factory ownership can be changed immediately and bypass timelock delay ast3ros - Vault Factory ownership can be changed immediately and bypass timelock delay May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants