hats-finance / Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb

Fork of the Inverter Smart Contracts Repository
GNU Lesser General Public License v3.0
0 stars 3 forks source link

Unauthorized access to `Governor_v1.acceptOwnership()` function which breaks protocol intended design #60

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x2889a8ca6e565def4db0ec6b469ec5dbacad47ea89830b304f2a62d328aa89c0 Severity: medium

Description: Description\ Governor_v1.acceptOwnership() is used to accept the ownership of other contracts if these contracts transfer their ownership to it. This is acheived by openzeppelin's ownable2step contract. Governor_v1.acceptOwnership() is implemented as:

    function acceptOwnership(address adr)
        external
        onlyCommunityOrTeamMultisig
    {
        if (adr.code.length == 0) {
            revert Governor__CallToTargetContractFailed();
        }

        (bool success,) =
            adr.call(abi.encodeCall(Ownable2Step.acceptOwnership, ()));

        // if the call is not a success
        if (!success) {
            revert Governor__CallToTargetContractFailed();
        }
        emit OwnershipAccepted(adr);
    }

acceptOwnership() can only be accessed by onlyCommunityOrTeamMultisig which means either COMMUNITY_MULTISIG_ROLE address OR TEAM_MULTISIG_ROLE address are allowed to accces it to accept the ownership of contracts.

but, the acceptOwnership() Natspec in interface states that only COMMUNITY_MULTISIG_ROLE is allowed to access it.

    /// @notice Accepts the ownership over the target address
@>  /// @dev can only be accessed by the COMMUNITY_MULTISIG_ROLE
    /// @param adr The address of target that wants to hand over the ownership
    function acceptOwnership(address adr) external;

Therefore, assuming the Natspec as Correct then acceptOwnership() function must be restricted to COMMUNITY_MULTISIG_ROLE address only.

Impact\ Unauthorized access to Governor_v1.acceptOwnership() function allows the Team multisig to access it which breaks protocol intended design as outlined in function Natspec.

Recommendation to fix\ Restrict access of Governor_v1.acceptOwnership()to COMMUNITY_MULTISIG_ROLE address only.

    function acceptOwnership(address adr)
        external
-        onlyCommunityOrTeamMultisig
+       onlyRole(COMMUNITY_MULTISIG_ROLE)
    {
        if (adr.code.length == 0) {
            revert Governor__CallToTargetContractFailed();
        }

        (bool success,) =
            adr.call(abi.encodeCall(Ownable2Step.acceptOwnership, ()));

        // if the call is not a success
        if (!success) {
            revert Governor__CallToTargetContractFailed();
        }
        emit OwnershipAccepted(adr);
    }
PlamenTSV commented 1 month ago

Low at best for breaking a Natspec, could be invalid if that's the new intention since the multisig cannot do any harm in accepting the ownership.

0xmahdirostami commented 1 month ago

@PlamenTSV Thanks.