Summary
The current implementation of the Adminable contract uses a single-step admin transfer process. This can lead to a loss of administrative control if a wrong address was passed during the transfer. This issue affects the protocol and in the scope of the contest, it affects SablierV2Lockup contract, which relies on the Adminable contract for administrative functions.
Vulnerability details
The Adminable contract allows for an immediate transfer of admin rights without requiring confirmation from the new admin. This can be risky if an incorrect address is provided, as the admin role could be lost forever.
Look at this:
function transferAdmin(address newAdmin) public virtual override onlyAdmin {
admin = newAdmin;
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
}
The SablierV2Lockup contract relies on the admin for critical functions such as setting the NFT descriptor. Losing the admin role can result in the inability to perform these admin tasks which can break protocol.
Look at the SablierV2Lockup.sol
function setNFTDescriptor(ISablierV2NFTDescriptor newNFTDescriptor) external override onlyAdmin {
ISablierV2NFTDescriptor oldNftDescriptor = nftDescriptor;
nftDescriptor = newNFTDescriptor;
emit ISablierV2Lockup.SetNFTDescriptor({
admin: msg.sender,
oldNFTDescriptor: oldNftDescriptor,
newNFTDescriptor: newNFTDescriptor
});
emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}
Impact
Losing the admin role can result in the inability to perform critical admin functions, potentially disrupting the functionality of the SablierV2Lockup contract.
Likelihood: Low, because it requires an error on the admin side
Impact: High, because protocol will be bricked
Hence, this is reported as Medium
Recommended Mitigation Steps:
Implement a two-step admin transfer pattern to ensure that the new admin explicitly accepts the role. This reduces the risk of losing admin control due to an incorrect address. Something like this:
Recommended Changes in Adminable.sol
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.22;
import { IAdminable } from "../interfaces/IAdminable.sol";
import { Errors } from "../libraries/Errors.sol";
/// @title Adminable
/// @notice See the documentation in {IAdminable}.
abstract contract Adminable is IAdminable {
/*//////////////////////////////////////////////////////////////////////////
STATE VARIABLES
//////////////////////////////////////////////////////////////////////////*/
/// @inheritdoc IAdminable
address public override admin;
+ /// @notice The address of the pending admin.
+ address public pendingAdmin;
/*//////////////////////////////////////////////////////////////////////////
MODIFIERS
//////////////////////////////////////////////////////////////////////////*/
/// @notice Reverts if called by any account other than the admin.
modifier onlyAdmin() {
if (admin != msg.sender) {
revert Errors.CallerNotAdmin({ admin: admin, caller: msg.sender });
}
_;
}
+ /// @notice Reverts if called by any account other than the pending admin.
+ modifier onlyPendingAdmin() {
+ if (pendingAdmin != msg.sender) {
+ revert Errors.CallerNotPendingAdmin({ pendingAdmin: pendingAdmin, caller: msg.sender });
+ }
+ _;
+ }
/*//////////////////////////////////////////////////////////////////////////
USER-FACING NON-CONSTANT FUNCTIONS
//////////////////////////////////////////////////////////////////////////*/
/// @inheritdoc IAdminable
- function transferAdmin(address newAdmin) public virtual override onlyAdmin {
- // Effect: update the admin.
- admin = newAdmin;
+ function transferAdmin(address newAdmin) public virtual override onlyAdmin {
+ // Effect: set the pending admin.
+ pendingAdmin = newAdmin;
// Log the pending transfer of the admin.
emit IAdminable.TransferAdmin({ oldAdmin: admin, newAdmin: newAdmin });
}
+ /// @notice Allows the pending admin to accept the role.
+ function acceptAdmin() public onlyPendingAdmin {
+ // Effect: update the admin.
+ admin = pendingAdmin;
+ pendingAdmin = address(0);
+ // Log the acceptance of the admin role.
+ emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: admin });
+ }
}
