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

Original admin can cancel airdrop streams

jennifersun

Summary

If the admin of the deployed SablierV2MerkleLT and SablierV2MerkleLL contracts is modified, the original admin may cancel airdrop streams.

Vulnerability Details

In SablierV2MerkleLT/SablierV2MerkleLL, when users claim, one stream will be created, and admin will be the created stream's sender. If SablierV2MerkleLockup/SablierV2MerkleLL transfers admin ownership. The existed created streams' sender is the original admin.

In readme, Sablier mentions one similar case in Known Issues

If the admin of the deployed SablierV2MerkleLT and SablierV2MerkleLL contracts is modified, the onLockupStreamWithdrawn() hook callback, if it is implemented, will continue to be made to the original admin for the airstreams that were already claimed at the time of the change.

Actually, if the admin is modified, the original owner can cancel existing streams if there streams are cancelable. The left funds will be returned back to the original admin.

function claim(
uint256 index,
address recipient,
uint128 amount,
bytes32[] calldata merkleProof
)
external
override
returns (uint256 streamId)
{
// Generate the Merkle tree leaf by hashing the corresponding parameters. Hashing twice prevents second
// preimage attacks.
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(index, recipient, amount))));
​
// Check: validate the function.
_checkClaim(index, leaf, merkleProof);
​
// Effect: mark the index as claimed.
_claimedBitMap.set(index);
​
// Interaction: create the stream via {SablierV2LockupLinear}.
streamId = LOCKUP_LINEAR.createWithDurations(
LockupLinear.CreateWithDurations({
@==> sender: admin,
recipient: recipient,
totalAmount: amount,
asset: ASSET,
cancelable: CANCELABLE,
transferable: TRANSFERABLE,
durations: streamDurations,
broker: Broker({ account: address(0), fee: ud(0) })
})
);
​
// Log the claim.
emit Claim(index, recipient, amount, streamId);
}
function _cancel(uint256 streamId) internal {
// Calculate the streamed amount.
uint128 streamedAmount = _calculateStreamedAmount(streamId);
​
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
​
// Calculate the sender's amount.
uint128 senderAmount;
unchecked {
senderAmount = amounts.deposited - streamedAmount;
}
​
// Calculate the recipient's amount.
uint128 recipientAmount = streamedAmount - amounts.withdrawn;
​
// Effect: mark the stream as canceled.
_streams[streamId].wasCanceled = true;
// Effect: make the stream not cancelable anymore, because a stream can only be canceled once.
_streams[streamId].isCancelable = false;
......
// Effect: set the refunded amount.
_streams[streamId].amounts.refunded = senderAmount;
​
// 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.
@==> transfer funds to original owner.
asset.safeTransfer({ to: sender, value: senderAmount });
......
}
​

Impact

Original admin can cancel existing streams which are claimed before the ownership changes.

Tools Used

Manual

Recommendations

Considering to limit the function of transfer ownership.

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.