Category: Coding Mistakes
The changeOwner function does not update the UPGRADER_ROLE
Low Severity
Low Impact
Low Likelihood
Description
The changeOwner function is used to change the owner of the contract. The function revokes the current owner's roles and grants the roles to the new owner. However, the UPGRADER_ROLE is not revoked from the current owner, nor is it granted to the new owner.
function changeOwner(address newOwnerAddress) public virtual onlyRole(DEFAULT_ADMIN_ROLE) {
_revokeRole(DEFAULT_ADMIN_ROLE, _msgSender());
_revokeRole(PAUSER_ROLE, _msgSender());
_revokeRole(MINTER_ROLE, _msgSender());
_revokeRole(WIPER_ROLE, _msgSender());
_revokeRole(KYC_ROLE, _msgSender());
// @audit-issue UPGRADER_ROLE is not revoked from the current owner, nor is it granted to the new owner
_grantRole(DEFAULT_ADMIN_ROLE, newOwnerAddress);
_grantRole(PAUSER_ROLE, newOwnerAddress);
_grantRole(MINTER_ROLE, newOwnerAddress);
_grantRole(WIPER_ROLE, newOwnerAddress);
_grantRole(KYC_ROLE, newOwnerAddress);
}Impact
This allows for the previous owner to still have the ability to upgrade the contract, even after transferring ownership.
Recommendations
We recommend revoking the UPGRADER_ROLE from the current owner and granting it to the new owner in the changeOwner function.
function changeOwner(address newOwnerAddress) public virtual onlyRole(DEFAULT_ADMIN_ROLE) {
_revokeRole(DEFAULT_ADMIN_ROLE, _msgSender());
// ...
+ _revokeRole(UPGRADER_ROLE, _msgSender());
_grantRole(DEFAULT_ADMIN_ROLE, newOwnerAddress);
// ...
+ _grantRole(UPGRADER_ROLE, newOwnerAddress);
}