search

code-423n4 / 2023-06-stader-findings

1 stars 1 forks source link

Risk of losing admin access if updateAdmin set with same current admin address #390

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

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.

Impact

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.

Proof of Concept

Contract : StaderConfig.sol Function : function updateAdmin(address _admin)

Using Brownie python automation framework commands in below examples.

Recommended Mitigation Steps

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
Picodes commented 1 year ago

"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?

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-sponsor commented 1 year ago

manoj9april marked the issue as sponsor confirmed

manoj9april commented 1 year ago

Sure, we will fix this.

c4-judge commented 1 year ago

Picodes marked issue #252 as primary and marked this issue as a duplicate of 252

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

rvierdiiev commented 1 year ago

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?

Co0nan commented 1 year ago

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.

Picodes commented 1 year ago

@rvierdiyev @Co0nan I respectfully disagree:

Picodes commented 1 year ago

@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.

Co0nan commented 1 year ago

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.

sanjay-staderlabs commented 1 year ago

This is fixed in the code