The admin address used in initialize function, can behave maliciously

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L102

Vulnerability details

N.B : This bug is different that the other one titled "Risk of losing admin access if updateAdmin set with same current admin address". Both issues are related to access control, but the impact, root cause and bug fix are different, so DO NOT mark it as dupliate of the other one.

The admin address used in the initialize function, retains DEFAULT_ADMIN_ROLE even after change to new admin using updateAdmin().

Impact

If the admin-A address that is used in the initialize function is changed to any new admin-B using updateAdmin(), still the original admin-A address retains DEFAULT_ADMIN_ROLE after updateAdmin(). So if the intent to change the initial admin-A is due to theft of private keys, etc., then there is a possibility that the original admin-A can still issue updateAdmin() to any new admin-C and act maliciously. Also admin-A still retains DEFAULT_ADMIN_ROLE and has full access to the StaderConfig contract, and act maliciously.

Proof of Concept

Contract : StaderConfig.sol Function : function initialize(address _admin, address _ethDepositContract)

Using Brownie python automation framework commands in below examples.

• Step#1 Contract Initialization During initialization, the admin-A address is granted DEFAULT_ADMIN_ROLE, but the setAccount() is not used, so accountsMap[ADMIN] = 0. Hence the value of StaderConfig.getAdmin() is 0.

• Step#2 update new Admin StaderConfig.updateAdmin(admin-B, {'from':admin-A})

When a new admin-B is set using above updateAdmin() command, the _revokeRole(DEFAULT_ADMIN_ROLE, oldAdmin) is ineffective. As a result in addition to the new admin-B, the initial admin-A still retains the DEFAULT_ADMIN_ROLE, and has access to all the functions with onlyRole(DEFAULT_ADMIN_ROLE) and can act maliciously. The value of StaderConfig.getAdmin() is admin-B One may think that admin-A loses DEFAULT_ADMIN_ROLE, but that is not the case.

• Step#3 Re-Issue same command Issuing the same above command once again StaderConfig.updateAdmin(admin-B, {'from':admin-A}) This will revoke the DEFAULT_ADMIN_ROLE access role of admin-B, due to another bug in updateAdmin() function, and admin-B loses all its admin access rights.

• Step#4 update another admin StaderConfig.updateAdmin(admin-C, {'from':admin-A})

The initial admin can once again use updateAdmin() to set any new admin-C

In summary, the initial admin-A never loses the DEFAULT_ADMIN_ROLE even after updateAdmin()

Recommended Mitigation Steps

Ref : https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L102 In the initialize function add this additional line after _grantRole

   _grantRole(DEFAULT_ADMIN_ROLE, _admin);
+  setAccount(ADMIN, _admin);

Assessed type

Access Control

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #171

[c4-judge]

Picodes marked the issue as satisfactory