Platform: Code4rena
Start Date: 02/06/2023
Pot Size: $100,000 USDC
Total HM: 15
Participants: 75
Period: 7 days
Judge: Picodes
Total Solo HM: 5
Id: 249
League: ETH
Rank: 12/75
Findings: 2
Award: $2,115.60
🌟 Selected for report: 1
🚀 Solo Findings: 0
1652.3182 USDC - $1,652.32
N.B : This bug is different that the other one titled "The admin address used in initialize function, can behave maliciously". 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.
Current admin will lose DEFAULT_ADMIN_ROLE role if updateAdmin issued with same address.
The is a possibility of loss of protocol admin access to this critical StaderConfig.sol contract, if updateAdmin() is set with same current admin address by mistake.
Contract : StaderConfig.sol Function : function updateAdmin(address _admin)
Using Brownie python automation framework commands in below examples.
Step#1 After initialization, admin-A is the admin which has the DEFAULT_ADMIN_ROLE
Step#2 update new Admin StaderConfig.updateAdmin(admin-B, {'from':admin-A}) The value of StaderConfig.getAdmin() is admin-B
Step#3 admin-B updates admin to itself again StaderConfig.updateAdmin(admin-B, {'from':admin-B}) The value of StaderConfig.getAdmin() is admin-B, but the DEFAULT_ADMIN_ROLE is revoked due to _revokeRole(DEFAULT_ADMIN_ROLE, oldAdmin); Now the protocol admin control is lost for StaderConfig contract
Ref : https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderConfig.sol#L177 In the updateAdmin() function, add a check for oldAdmin != _admin , like below
address oldAdmin = accountsMap[ADMIN]; + require(oldAdmin != _admin, "Already set to admin"); ## Assessed type Access Control
#0 - Picodes
2023-06-10T13:23:41Z
"so DO NOT mark it as dupliate of the other one." -> I don't understand, is this an order (in which case it is a bit inappropriate), or a discussion between the warden and someone?
#1 - c4-judge
2023-06-10T13:24:11Z
Picodes changed the severity to 2 (Med Risk)
#2 - c4-judge
2023-06-12T12:53:22Z
Picodes marked the issue as primary issue
#3 - c4-sponsor
2023-06-21T12:59:08Z
manoj9april marked the issue as sponsor confirmed
#4 - manoj9april
2023-06-21T12:59:25Z
Sure, we will fix this.
#5 - c4-judge
2023-07-02T09:42:57Z
Picodes marked issue #252 as primary and marked this issue as a duplicate of 252
#6 - c4-judge
2023-07-02T09:43:12Z
Picodes marked the issue as selected for report
#7 - rvierdiyev
2023-07-04T07:25:44Z
Isn't this same as just transfer role to the address 0 or any other address? Why protocol will need to change role to same address? Isn't this informative issue?
#8 - Co0nan
2023-07-04T12:55:22Z
I believe this fall under the "Admin Privilege" category as such an issue should be marked as QA based on C4 docs and how similar issues got judged.
#9 - Picodes
2023-07-04T23:22:11Z
@rvierdiyev @Co0nan I respectfully disagree:
initialize there is no call to setAccount(ADMIN, _admin); (see https://github.com/code-423n4/2022-06-stader-findings/issues/133) then it becomes actually likely that the admin calls this function for himself#10 - Picodes
2023-07-04T23:24:56Z
@Co0nan for information "Admin Privilege" aren't always QA, it depends on the context and is up to the judge. See https://github.com/code-423n4/org/issues/54 and https://github.com/code-423n4/org/issues/59 for the ongoing discussion about this.
#11 - Co0nan
2023-07-04T23:41:12Z
Here there is clearly a slight bug in the code.
@Picodes personally I got my reports downgraded to QA even though the bug was clearly in the code itself but because it only occurs through an action taken by a privileged user it's downgraded. Here, this bug occurs from Admin as he passes an address twice. I have to stand with @rvierdiyev this is likely to missing Zero address Check on onlyOwner functions.
However, It's up to you as the Judge and I respect your final conclusion.
#12 - sanjay-staderlabs
2023-07-13T04:17:05Z
This is fixed in the code
463.2846 USDC - $463.28
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().
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.
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()
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);
Access Control
#0 - c4-judge
2023-06-10T13:25:53Z
Picodes changed the severity to 2 (Med Risk)
#1 - c4-judge
2023-06-10T13:25:58Z
Picodes marked the issue as primary issue
#2 - c4-judge
2023-06-10T13:26:58Z
Picodes marked the issue as duplicate of #171
#3 - c4-judge
2023-07-02T12:45:25Z
Picodes marked the issue as satisfactory