Summary
In SablierV2Lockup.sol, cancel and renounce function has the same condition logic, It’s recommended to use the modifier which make the code clearer and less gas costing.
Vulnerability Details
In renounce function, Before the actual renouce is performed, the status of steam and the caller is checked.The conditions include:
if the steam status is DEPLETED
if the steam status is CANCELED
if the steam status is SETTLED
if the caller is SENDER
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L284-L301
function renounce(uint256 streamId) external override noDelegateCall notNull(streamId) updateMetadata(streamId) {
Lockup.Status status = _statusOf(streamId);
if (status == Lockup.Status.DEPLETED) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
} else if (status == Lockup.Status.CANCELED) {
revert Errors.SablierV2Lockup_StreamCanceled(streamId);
} else if (status == Lockup.Status.SETTLED) {
revert Errors.SablierV2Lockup_StreamSettled(streamId);
}
if (!_isCallerStreamSender(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
_renounce(streamId);
Similarly, In cancel function, Before the actual cancel is performed, the status of steam and the caller is checked.The conditions include:
if the steam status is DEPLETED
if the steam status is CANCELED
if the steam status is SETTLED. (there is checked in _cancel function)
if the caller is SENDER
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L256C2-L271C6
https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L559C5-L561C10
function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) {
if (_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
} else if (_streams[streamId].wasCanceled) {
revert Errors.SablierV2Lockup_StreamCanceled(streamId);
}
if (!_isCallerStreamSender(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
_cancel(streamId);
}
function _cancel(uint256 streamId) internal {
uint128 streamedAmount = _calculateStreamedAmount(streamId);
Lockup.Amounts memory amounts = _streams[streamId].amounts;
if (streamedAmount >= amounts.deposited) {
revert Errors.SablierV2Lockup_StreamSettled(streamId);
}
if (!_streams[streamId].isCancelable) {
revert Errors.SablierV2Lockup_StreamNotCancelable(streamId);
}
So, It’s recommended to use the modifier include all the same condition checking, which make the code clearer and less gas costing.
Impact
so as to favor simplicity, avoid confusions and reduce the code’s attack surface, consider removing the unnecessary code repetition.
Tools Used
Recommendations
add the modifier and modify the cancel and renounce function code.
modifier CanCancelOrRenouce(uint256 streamId) {
Lockup.Status status = _statusOf(streamId);
if (status == Lockup.Status.DEPLETED) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
} else if (status == Lockup.Status.CANCELED) {
revert Errors.SablierV2Lockup_StreamCanceled(streamId);
} else if (status == Lockup.Status.SETTLED) {
revert Errors.SablierV2Lockup_StreamSettled(streamId);
}
if (!_isCallerStreamSender(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
_;
}
function cancel(uint256 streamId) public override noDelegateCall CanCancelOrRenouce notNull(streamId) {
_cancel(streamId);
}
function renounce(uint256 streamId) external override noDelegateCall CanCancelOrRenouce notNull(streamId) updateMetadata(streamId) {
