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

if admin is modified then the protocol should allow the new admin to cancel/renounce stream

0xaman

Summary

The Adminable contract allow the current admin to transfer ownership to other address. However there are two issue associated with it 1). The new admin is not able to cancel the stream or renounce stream. 2). The old admin can cancel/renounce the old stream.

Vulnerability Details

When ever we create the new stream the current admin is set has a sender of stream.

SablierV2MerkleLL.sol:79
79: // Interaction: create the stream via {SablierV2LockupLinear}.
80: streamId = LOCKUP_LINEAR.createWithDurations(
81: LockupLinear.CreateWithDurations({
82: sender: admin,
83: recipient: recipient,
84: totalAmount: amount,
85: asset: ASSET,
86: cancelable: CANCELABLE,
87: transferable: TRANSFERABLE,
88: durations: streamDurations,
89: broker: Broker({ account: address(0), fee: ud(0) })
90: })
91: );
​

At Line 82 the current admin is set as sender of stream. The admin can also be changed to another address

Adminable.sol:34
34: function transferAdmin(address newAdmin) public virtual override onlyAdmin {
35: // Effect: update the admin.
36: admin = newAdmin;
37:
38: // Log the transfer of the admin.
39: emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
40: }
​

transferAdmin function is most of the time used for admin rotation or when admin private key got leaked, Private Key leak is currently 2nd most hacks happen in web3.
if Private key leak happens the team will change the admin to new address and make protocol safe.
Now let have a look what this admin/sender address is allowed to do.

  1. it can cancel the stream. upon canceling the stream amount left will be sent to the sender who has created this stream.

function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) {
// Check: the stream is neither depleted nor canceled.
...
// Check: `msg.sender` is the stream's sender.
if (!_isCallerStreamSender(streamId)) { // <---------------- old admin only
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Checks, Effects and Interactions: cancel the stream.
_cancel(streamId);
}
function _cancel(uint256 streamId) internal {
...
// Retrieve the sender and the recipient from storage.
address sender = _streams[streamId].sender;
address recipient = _ownerOf(streamId);
​
// Retrieve the ERC-20 asset from storage.
IERC20 asset = _streams[streamId].asset;
​
// Interaction: refund the sender.
// asset.safeTransfer({ to: sender, value: senderAmount });
asset.safeTransfer({ to: sender, value: senderAmount }); //<---------- asset transfer to the old admin
...
}

  1. renounce the stream

SablierV2Lockup.sol:261
261: function renounce(uint256 streamId) external override noDelegateCall notNull(streamId) updateMetadata(streamId) {
262: // Check: the stream is not cold.
....
272: // Check: `msg.sender` is the stream's sender.
273: if (!_isCallerStreamSender(streamId)) {
274: revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
275: }
276:
277: // Checks and Effects: renounce the stream.
278: _renounce(streamId);
279:
...
289: }
​

the Issue here is most of the time the Private key Leaks are handle via changing the admin , here after changing the admin the old one is still able to cancel the stream and steal the funds from stream.
Note: This issue is not added to known Issues , The known issue only state about the callback call. and also the Protocol assumes that all the user will be using safe for admin role which is not correct most of the time.

Impact

The Impact will be high if Private key hack occur Because the old admin can still cancel all the stream and steal the funds lock in stream.

Tools Used

Manual Review

Recommendations

either allow both old and new Admin to cancel and renounce stream or add it known issues. and you can also block the old admin to cancel stream. it is debatable that why i am not suggesting a single fix.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Admin changing functionality allows former admin access and does not give new admin access to some functionality

0xnevi Judge
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Admin changing functionality allows former admin access and does not give new admin access to some functionality

Support

FAQs

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