Summary
Although the README.md states that 'These are callers who are neither Sender nor Recipient but are allowed to trigger withdrawals on behalf of the recipients. This is because the withdraw function is publicly callable.'
The root cause is not only publicly callable but in the if statement.
if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
Update above if statement could further limit unknown callers and their unexpected behaviour and fulfil the documentation (https://docs.sablier.com/contracts/v2/reference/access-control):
"The assets in a stream can be withdrawn by the sender, recipient, or an approved NFT operator.
Both the recipient and the NFT operator have the option to specify a custom address to withdraw the assets to.
The sender, however, is limited to withdrawing assets directly to the recipient's address."
Vulnerability Details
In SablierV2Lockup::withdraw, there is a check to ensure
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L361-L365
The withdrawal address must be the recipient AND
msg.sender is neither the stream's recipient nor an approved third party
it requires both conditions to be 'True' && 'True' to make the check below revert
if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
To look closer at the _isCallerStreamRecipientOrApproved, the function is to check whether msg.sender is the stream's recipient or an approved third party. If not, then return false.
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L468-L472
function _isCallerStreamRecipientOrApproved(uint256 streamId) internal view returns (bool) {
address recipient = _ownerOf(streamId);
return msg.sender == recipient || isApprovedForAll({ owner: recipient, operator: msg.sender })
|| getApproved(streamId) == msg.sender;
}
So under a situation: msg.sender is a malicious user, the malicious user checks the streams' status and finds streams are not depleted. And recipient is address Mary and no approved NFT operator.
to = address(Mary)
recipient = address(Mary)
Result: to != recipient --> False
msg.sender = Malicious User
Result1: isCallerStreamRecipientOrApproved --> False
Result2: !isCallerStreamRecipientOrApproved --> True
Check above does not revert because of 'False' && 'True'.
POC
POC1:
Update the SablierV2Lockup::isCallerStreamRecipientOrApproved to external/public and add below test check code in LockupLinear.t.sol::test_Withdraw_CallerUnknownAddress
function test_Withdraw_CallerUnknownAddress()
external
whenNotDelegateCalled
givenNotNull
givenStreamNotDepleted
whenToNonZeroAddress
whenWithdrawAmountNotZero
whenNoOverdraw
whenWithdrawalAddressIsRecipient
{
resetPrank({ msgSender: address(0xCAFE) });
address addressTo = users.recipient;
address addressRecipient = users.recipient;
bool toNotEqualRecipient = addressTo != addressRecipient;
assertEq(toNotEqualRecipient, false, "to != recipient");
console.log("to != recipient:", toNotEqualRecipient);
bool isCallerStreamRecipientOrApproved = lockup.isCallerStreamRecipientOrApproved(defaultStreamId);
assertEq(isCallerStreamRecipientOrApproved , false, "isCallerStreamRecipientOrApproved");
console.log("isCallerStreamRecipientOrApproved:", isCallerStreamRecipientOrApproved);
bool notIsCallerStreamRecipientOrApproved = !isCallerStreamRecipientOrApproved;
assertEq(notIsCallerStreamRecipientOrApproved , true, "!_isCallerStreamRecipientOrApproved");
console.log("!isCallerStreamRecipientOrApproved:", notIsCallerStreamRecipientOrApproved);
console.log("to != recipient && !isCallerStreamRecipientOrApproved:", toNotEqualRecipient && notIsCallerStreamRecipientOrApproved);
vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() });
lockup.withdraw({ streamId: defaultStreamId, to: users.recipient, amount: defaults.WITHDRAW_AMOUNT() });
uint128 actualWithdrawnAmount = lockup.getWithdrawnAmount(defaultStreamId);
uint128 expectedWithdrawnAmount = defaults.WITHDRAW_AMOUNT();
assertEq(actualWithdrawnAmount, expectedWithdrawnAmount, "withdrawnAmount");
}
Result
[PASS] test_Withdraw_CallerUnknownAddress() (gas: 76881)
Logs:
to != recipient: false
isCallerStreamRecipientOrApproved: false
!isCallerStreamRecipientOrApproved: true
to != recipient && !isCallerStreamRecipientOrApproved: false
POC2:
Split the check logic in SablierV2Lockup.sol::withdraw and get the !_isCallerStreamRecipientOrApproved check from function burn
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L247-L249
if (to != recipient) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
if (!isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
Result:
Failing tests:
Encountered 1 failing test in test/integration/concrete/lockup-dynamic/LockupDynamic.t.sol:Withdraw_LockupDynamic_Integration_Concrete_Test
[FAIL. Reason: SablierV2Lockup_Unauthorized(1, 0x000000000000000000000000000000000000cafE)] test_Withdraw_CallerUnknownAddress() (gas: 41416)
Encountered 1 failing test in test/integration/concrete/lockup-linear/LockupLinear.t.sol:Withdraw_LockupLinear_Integration_Concrete_Test
[FAIL. Reason: SablierV2Lockup_Unauthorized(1, 0x000000000000000000000000000000000000cafE)] test_Withdraw_CallerUnknownAddress() (gas: 41451)
Encountered 1 failing test in test/integration/concrete/lockup-tranched/LockupTranched.t.sol:Withdraw_LockupTranched_Integration_Concrete_Test
[FAIL. Reason: SablierV2Lockup_Unauthorized(1, 0x000000000000000000000000000000000000cafE)] test_Withdraw_CallerUnknownAddress() (gas: 41416)
Impact
Malicioius user can SablierV2Lockup::withdraw and may cause unexpected withdrawals.
Tools Used
Manual & Foundry
Recommendations
SablierV2Lockup.sol::withdraw
address sender = _streams[streamId].sender;
if (msg.sender == sender) {
if (to != recipient) {
revert Errors.SablierV2Lockup_SenderWithdrawalAddressNotRecipient(streamId, recipient, to);
}
} else if (!_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_UserNotAllowedToWithdraw(streamId, msg.sender);
}
Error.sol
error SablierV2Lockup_SenderWithdrawalAddressNotRecipient(uint256 streamId, address recipient, address to);
error SablierV2Lockup_UserNotAllowedToWithdraw(uint256 streamId, address caller);
