Skip to content

Latest commit

 

History

History
144 lines (98 loc) · 5.87 KB

050.md

File metadata and controls

144 lines (98 loc) · 5.87 KB

Glamorous Canvas Camel

Medium

Improper Use of getRoleMember in updateOwner May Lead to Incorrect Owner Role Revocation

Summary

The ReputationMarket contract, which inherits from AccessControl, uses the updateOwner function to transfer ownership. This function relies on getRoleMember(OWNER_ROLE, 0) to identify the current owner for role revocation. However, the AccessControlEnumerableUpgradeable contract from OpenZeppelin specifies that role members are not stored in any particular order, and their ordering may change. This means that getRoleMember(OWNER_ROLE, 0) may not reliably return the current owner (msg.sender), leading to improper revocation of the owner role from unintended addresses. This can result in ownership mismanagement, unauthorized access, or loss of control over sensitive functions in the ReputationMarket contract.

Root Cause

the updateOwner function is implemented as follows:

https://github.com/sherlock-audit/2024-12-ethos-update/blob/c3a2b007d0ddfcb476f300f8b766808f0e3e2dfd/ethos/packages/contracts/contracts/ReputationMarket.sol#L4

https://github.com/sherlock-audit/2024-12-ethos-update/blob/c3a2b007d0ddfcb476f300f8b766808f0e3e2dfd/ethos/packages/contracts/contracts/utils/AccessControl.sol#L106C1-L109C4

function updateOwner(address owner) external onlyOwner {
    _revokeRole(OWNER_ROLE, getRoleMember(OWNER_ROLE, 0));
    _grantRole(OWNER_ROLE, owner);
}

This function assumes that:

  1. There is only one address with the OWNER_ROLE.
  2. The owner is always at index 0 in the list of role members.

However, the AccessControlEnumerableUpgradeable contract (from OpenZeppelin) used by AccessControl manages role members using an EnumerableSet, which does not guarantee any specific ordering of elements. The relevant part of the OpenZeppelin code is:

/**
 * @dev Returns one of the accounts that have `role`. `index` must be a
 * value between 0 and {getRoleMemberCount}, non-inclusive.
 *
 * Role bearers are not sorted in any particular way, and their ordering may
 * change at any point.
 *
 * WARNING: When using {getRoleMember} and {getRoleMemberCount}, make sure
 * you perform all queries on the same block. See the following
 * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
 * for more information.
 */
function getRoleMember(bytes32 role, uint256 index) public view virtual returns (address) {
    AccessControlEnumerableStorage storage $ = _getAccessControlEnumerableStorage();
    return $._roleMembers[role].at(index);
}

The key point is that "Role bearers are not sorted in any particular way, and their ordering may change at any point."

Therefore, using getRoleMember(OWNER_ROLE, 0) does not reliably return the current owner (msg.sender). If multiple addresses have the OWNER_ROLE, the address at index 0 may not be the caller. This can lead to revoking the owner role from an unintended address.

Internal Pre-conditions

No response

External Pre-conditions

No response

Attack Path

  1. Multiple Owners: Assume multiple addresses hold the OWNER_ROLE due to prior role assignments.

  2. Ownership Transfer Attempt: An owner (Owner1) calls updateOwner(newOwnerAddress) to transfer ownership.

  3. Incorrect Role Revocation: The function revokes the OWNER_ROLE from the address at index 0, which might not be Owner1, but another owner (Owner2).

  4. Ownership Mismanagement:

    • Owner1 still retains the OWNER_ROLE.
    • Owner2 loses the OWNER_ROLE unexpectedly.
    • The new owner (newOwnerAddress) gains the OWNER_ROLE.

  5. Potential Unauthorized Access: Unintended addresses may retain owner privileges, leading to unauthorized access to sensitive functions in ReputationMarket, such as upgrading the contract via _authorizeUpgrade:

    function _authorizeUpgrade(
    address newImplementation
) internal override onlyOwner onlyNonZeroAddress(newImplementation) {
    // Intentionally left blank to ensure onlyOwner and zeroCheck modifiers run
}

Impact

  • Ownership Mismanagement: Ownership transfer may not work as intended, leading to multiple owners or loss of control by the intended owner.
  • Unauthorized Access: Addresses that should no longer have the OWNER_ROLE may retain it, allowing them to perform sensitive operations.

PoC

  1. Setup: Two addresses, Owner1 and Owner2, hold the OWNER_ROLE:

    // Both Owner1 and Owner2 have been granted OWNER_ROLE

  2. Ownership Transfer Attempt: Owner1 tries to transfer ownership to NewOwner:

    reputationMarket.updateOwner(newOwnerAddress);

  3. Function Execution in AccessControl:

    function updateOwner(address owner) external onlyOwner {
    // Revokes OWNER_ROLE from the address at index 0 (could be Owner2)
    _revokeRole(OWNER_ROLE, getRoleMember(OWNER_ROLE, 0));
    _grantRole(OWNER_ROLE, owner); // Grants OWNER_ROLE to NewOwner
}

  4. Outcome:

    • Owner2 (at index 0) loses the OWNER_ROLE.
    • Owner1 retains the OWNER_ROLE.
    • NewOwner gains the OWNER_ROLE.
    • This results in Owner1 and NewOwner both having the OWNER_ROLE, which may not be intended.

  5. Resulting Issues:

    • Owner1 did not lose their owner privileges.
    • Owner2 lost ownership unintentionally.
    • Potential for unauthorized actions by unintended owners.

Mitigation

Modify the updateOwner function to revoke the OWNER_ROLE from the caller (msg.sender) instead of using getRoleMember:

function updateOwner(address owner) external onlyOwner {
    _revokeRole(OWNER_ROLE, msg.sender);
    _grantRole(OWNER_ROLE, owner);
}

This ensures that the owner transferring ownership loses their privileges, and the new owner gains them.