Owner should not renounce itself

Summary

The VestingEscrowFactory contract depends on the owner to for many things, including updatingValidator, changeManager etc. More importantly, recovered funds are transferred to the owner address as seen here:

Therefore, it is imperative that the owner address at no point is reset to 0 or renounced.

Now, the VestingEscrowFactory contract inherits from Ownable2Step contract as seen here: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L13 which allows the owner to renounce itself. I have provided the below PoC showing the owner renouncing ownership and setting it to address(0).

With the vitality of the owner() address, it is a huge security risk that can lead to loss of funds if it is renounced.

Vulnerability Detail

As seen in this PoC code that can be added to the VestingEscrowFactory contract

    function testRenounceOwner() public {
        vm.startPrank(address(factory.owner()));
        console.logAddress(factory.owner());
        factory.renounceOwnership();
        console.logAddress(factory.owner());
    }

It is possible to renounce ownership.

Impact

Funds recovery would be impossible leading to Permanently stuck funds

Code Snippet

Tool used

Manual Review

Recommendation

Consider overriding the Ownable2Step renounce function and make the call revert.

nevillehuang commented 9 months ago

Invalid, admins are trusted entities to not simply renounce contracts. See point 5. of sherlock rules

sherlock-admin2 commented 9 months ago

1 comment(s) were left on this issue during the judging contest.

pratraut commented:

'invalid due to owner being TRUSTED entity'

sherlock-audit / 2024-01-rio-vesting-escrow-judging

0x_Scar - Owner should not renounce itself #69