Description

To execute the necessary transactions, the contract owner has the ability to assign accounts to one of two roles, Minter or Redeemer, with the registerAllowedRole function. This function sets the trusted signer address for the account and increments the _roleActiveSignerCount.

function registerAllowedRole(
  Role role,
  address account,
  address signer,
  bool allowed
) external override nonReentrant onlyOwner {
  ...
  _roleRegistry[role][account][signer] = allowed;
  _roleActiveSignerCount[role][account] += 1;
  ...
  }

Further, this value _roleActiveSignerCount is used to validate whether the account is assigned the appropriate role. To pass the verification, the account must have at least one confirmed signer.

function isMinter(address minter) public view returns (bool) {
  return _roleActiveSignerCount[Role.Minter][minter] >= 1;
}

Accounts with a role can also add their own trusted signer accounts using the registerAllowedSigner function and remove them with the removeRole function, which deletes the _roleRegistry[role][msg.sender][signer] and decrements _roleActiveSignerCount for the msg.sender.

function registerAllowedSigner(
  Role role,
  address signer,
  bool allowed
) external override nonReentrant onlyRoleOrOwner(role) {
  if (signer == address(0)) revert InvalidAddress();
  _roleRegistry[role][msg.sender][signer] = allowed;
  _roleActiveSignerCount[role][msg.sender] += 1;
  emit RoleRegistered(role, msg.sender, msg.sender, allowed);
}

function removeRole(Role role, address signer) public nonReentrant onlyRoleOrOwner(role) {
  if (!_roleRegistry[role][msg.sender][signer]) revert InvalidRole();
  delete _roleRegistry[role][msg.sender][signer];
  _roleActiveSignerCount[role][msg.sender] -= 1;
  emit RoleRemoved(role, signer);
}

Impact

Since functions registerAllowedRole and registerAllowedSigner do not have checks that the signer has already been added to the account, the _roleActiveSignerCount can be incremented repeatedly without adding a new signer. And in this case, during the removeRole function call, the _roleActiveSignerCount for the account will never reach zero; as a result, the account will always be assigned with this role, even if the account does not have allowed signers.

In addition, the registerAllowedRole and registerAllowedSigner functions get the input bool allowed argument, which determines whether the signer address will be set as trusted. In case allowed is equal to false, the _roleActiveSignerCount will still be increased, although the signer address was not actually added. So it will lead to the incorrect counting of _roleActiveSignerCount.

Recommendations

We recommend considering using the OpenZeppelin's AccessControl↗ module because it provides a secure, audited, and standards-compliant way to manage roles and permissions and reduces the potential security risks associated with creating a custom solution.

Remediation

Ethena Labs acknowledged this finding and implemented a fix in commit df19a0d3↗.