Sablier
Sablier
DeFiFoundry
53,440 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Adminable : transferAdmin is not done in two step

ak1

Summary

The admin rights for sablier is transferred in single action.

Vulnerability Details

Single-step ownership transfer means that if a wrong address was passed when transferring ownership or admin rights it can mean that role is lost forever. The ownership pattern implementation for the protocol is in OwnableUpgradeable.sol where a single-step transfer is implemented.This can be a problem for all methods marked in onlyOwner throughout the protocol, some of which are core protocol functionality.

The transferAdmin function transfer admin right in single call.

https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/Adminable.sol#L33-L40

/// @inheritdoc IAdminable
function transferAdmin(address newAdmin) public virtual override onlyAdmin {
// Effect: update the admin.
admin = newAdmin;
​
// Log the transfer of the admin.
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
}

Impact

High, because important protocol functionality will be bricked

Tools Used

Manual review.

Recommendations

It is a best practice to use two-step ownership transfer pattern, meaning ownership transfer gets to a "pending" state and the new owner should claim his new rights, otherwise the old owner still has control of the contract. Consider using OpenZeppelin's Ownable2Step contract

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.