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

Critical changes should use two-step procedure

bube

Summary

The Adminable contract allows the current admin to transfer admin rights to a new address using the transferAdmin function. However, this function lacks a two-step procedure, which can lead to potential security risks and operational issues.

Vulnerability Details

The Adminable::transferAdmin function is used to transfer the admin role from the current admin to the new admin:

​
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 });
}
​

And the function directly updates the admin address with the new one provided as input argument. But a copy-paste error or a typo in the address of the new admin can end up bricking protocol functionality and leaving the protocol without admin.
Also, there is a test that shows the succefully transfer of admin rights to the zero address in the test file transferAdmin.t.sol:

​
function test_TransferAdmin_ZeroAddress() external whenCallerAdmin {
// Expect the relevant event to be emitted.
vm.expectEmit({ emitter: address(adminableMock) });
emit TransferAdmin({ oldAdmin: users.admin, newAdmin: address(0) });
​
// Transfer the admin.
adminableMock.transferAdmin(address(0));
​
// Assert that the admin has been transferred.
address actualAdmin = adminableMock.admin();
address expectedAdmin = address(0);
assertEq(actualAdmin, expectedAdmin, "admin");
}
​

I know that checking the zero address is invalid according to the documentation, but this should be prevented because if admin rights are transferred to the zero address, the protocol will remain adminless.

Impact

The current admin might accidentally transfer admin rights to an incorrect address without any confirmation step, leading to potential loss of control over the contract.

Tools Used

Manual Review

Recommendations

Implement a two-step procedure for transferring admin rights. The process typically involves:

  1. Initiation: The current admin initiates the transfer by specifying the new admin address.

  2. Confirmation: The new admin address must confirm the transfer within a certain period.

address public override admin;
+ address public pendingAdmin;
​
/// @notice Reverts if called by any account other than the admin.
modifier onlyAdmin() {
//checks if the admin is a msg.sender and reverts
if (admin != msg.sender) {
revert Errors.CallerNotAdmin({ admin: admin, caller: msg.sender });
}
_;
}
​
+ modifier onlyPendingAdmin() {
+ if (pendingAdmin != msg.sender) {
+ revert Errors.CallerNotPendingAdmin({ pendingAdmin: pendingAdmin, caller: msg.sender });
+ }
+ _;
+ }
​
- 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 });
- }
​
+ function initiateTransferAdmin(address newAdmin) public onlyAdmin {
+ require(newAdmin != address(0), "New admin address cannot be zero");
+ pendingAdmin = newAdmin;
+ emit IAdminable.InitiateTransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
+ }
​
+ function confirmTransferAdmin() public onlyPendingAdmin {
+ admin = pendingAdmin;
+ pendingAdmin = address(0);
+ emit IAdminable.TransferAdmin({ oldAdmin: admin, newAdmin: msg.sender });
+ }
​

By separating the initiation and confirmation steps, the risk of accidental transfers is significantly reduced. The current admin cannot inadvertently transfer the role without the new admin's confirmation.
Another solution is to use the Ownable2Step from Openzeppelin.

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.