Submission Details
Severity:
Self Transfer Bug In SablierV2Lockup::withdrawMaxAndTransfer
Summary
The withdrawMaxAndTransfer function in SablierV2Lockup contains a bug that allows self-transfers of NFT to the same address, which leads to a disruption in the contract's logic and repeated self-transfer of NFTs to oneself.
Vulnerability Details
vulnerability arises because the withdrawMaxAndTransfer function does not include a check to prevent the transfer of an NFT to the current recipient (self-transfer). The function allows the current recipient to withdraw the maximum available amount from a stream and transfer the NFT associated with the stream to a new recipient. However, the lack of a self-transfer check can lead to the following scenario:
The current recipient (e.g., Alice) calls withdrawMaxAndTransfer with themselves as the new recipient.
The function does not prevent this self-transfer and proceeds to execute it.
Function logs the withdrawal and transfer of NFT to the new recipient in the transaction causing misconfiguration in the function's transfer logic.
/// @inheritdoc ISablierV2Lockup
function withdrawMaxAndTransfer(
uint256 streamId,
address newRecipient
)
external
override
noDelegateCall
notNull(streamId)
{
// Check: the caller is the current recipient. This also checks that the NFT was not burned.
address currentRecipient = _ownerOf(streamId);
if (msg.sender != currentRecipient) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
// Skip the withdrawal if the withdrawable amount is zero.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (withdrawableAmount > 0) {
withdraw({ streamId: streamId, to: currentRecipient, amount: withdrawableAmount });
}
// Checks and Effects: transfer the NFT.
_transfer({ from: currentRecipient, to: newRecipient, tokenId: streamId }); <@ Found
}
Coded PoC
Test case for transferring NFT to same recipient :
paste this test in withdrawMaxAndTransfer.t.sol and run forge test --mt test_WithdrawMaxAndTransfer_SameRecipient -vvvv
function test_WithdrawMaxAndTransfer_SameRecipient()
external
whenNotDelegateCalled
givenNotNull
whenCallerCurrentRecipient
givenNFTNotBurned
givenWithdrawableAmountNotZero
givenStreamTransferable
{
// Simulate the passage of time.
vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() });
// Get the withdraw amount.
uint128 withdrawAmount = lockup.withdrawableAmountOf(defaultStreamId);
// Expect the assets to be transferred to the Recipient.
expectCallToTransfer({ to: users.recipient, value: withdrawAmount });
// Expect the relevant events to be emitted.
vm.expectEmit({ emitter: address(lockup) });
emit WithdrawFromLockupStream({
streamId: defaultStreamId,
to: users.recipient,
amount: withdrawAmount,
asset: dai
});
vm.expectEmit({ emitter: address(lockup) });
emit Transfer({ from: users.recipient, to: users.recipient, tokenId: defaultStreamId });
vm.expectEmit({ emitter: address(lockup) });
emit MetadataUpdate({ _tokenId: defaultStreamId });
// Make the max withdrawal and transfer the NFT.
lockup.withdrawMaxAndTransfer({ streamId: defaultStreamId, newRecipient: users.recipient });
// Assert that the withdrawn amount has been updated.
uint128 actualWithdrawnAmount = lockup.getWithdrawnAmount(defaultStreamId);
uint128 expectedWithdrawnAmount = withdrawAmount;
assertEq(actualWithdrawnAmount, expectedWithdrawnAmount, "withdrawnAmount");
// Assert that the recipient is still the same.
address actualRecipient = lockup.getRecipient(defaultStreamId);
address expectedRecipient = users.recipient;
assertEq(actualRecipient, expectedRecipient, "recipient");
}
}
And results of Testcases passing (verbosity = 4)
forge test --mt test_WithdrawMaxAndTransfer_SameRecipient -vvvv
[⠒] Compiling...
No files changed, compilation skipped
Ran 1 test for test/integration/concrete/lockup-dynamic/LockupDynamic.t.sol:WithdrawMaxAndTransfer_LockupDynamic_Integration_Concrete_Test
[PASS] test_WithdrawMaxAndTransfer_SameRecipient() (gas: 136108)
Traces:
[136108] WithdrawMaxAndTransfer_LockupDynamic_Integration_Concrete_Test::test_WithdrawMaxAndTransfer_SameRecipient()
├─ [273] Defaults::WARP_26_PERCENT() [staticcall]
│ └─ ← [Return] 1714693400 [1.714e9]
├─ [0] VM::warp(1714693400 [1.714e9])
│ └─ ← [Return]
├─ [30405] LockupDynamic::withdrawableAmountOf(1) [staticcall]
│ └─ ← [Return] 3366025403784438632500 [3.366e21]
├─ [0] VM::expectCall(DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xa9059cbb00000000000000000000000003e9b88f4b1406163ef9ec4875a52e1e55953ec10000000000000000000000000000000000000000000000b678fc7ec668425034)
│ └─ ← [Return]
├─ [0] VM::expectEmit(LockupDynamic: [0x49c3486EC9f488230dD85FAc098929AeA9a3A898])
│ └─ ← [Return]
├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 3366025403784438632500 [3.366e21])
├─ [0] VM::expectEmit(LockupDynamic: [0x49c3486EC9f488230dD85FAc098929AeA9a3A898])
│ └─ ← [Return]
├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
├─ [0] VM::expectEmit(LockupDynamic: [0x49c3486EC9f488230dD85FAc098929AeA9a3A898])
│ └─ ← [Return]
├─ emit MetadataUpdate(_tokenId: 1)
├─ [74946] LockupDynamic::withdrawMaxAndTransfer(1, Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1])
│ ├─ [12866] DAI::transfer(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], 3366025403784438632500 [3.366e21])
│ │ ├─ emit Transfer(from: LockupDynamic: [0x49c3486EC9f488230dD85FAc098929AeA9a3A898], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], value: 3366025403784438632500 [3.366e21])
│ │ └─ ← [Return] true
│ ├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 3366025403784438632500 [3.366e21])
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ ├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ └─ ← [Stop]
├─ [845] LockupDynamic::getWithdrawnAmount(1) [staticcall]
│ └─ ← [Return] 3366025403784438632500 [3.366e21]
├─ [0] VM::assertEq(3366025403784438632500 [3.366e21], 3366025403784438632500 [3.366e21], "withdrawnAmount") [staticcall]
│ └─ ← [Return]
├─ [665] LockupDynamic::getRecipient(1) [staticcall]
│ └─ ← [Return] Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1]
├─ [0] VM::assertEq(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], "recipient") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.15ms (344.68µs CPU time)
Ran 1 test for test/integration/concrete/lockup-tranched/LockupTranched.t.sol:WithdrawMaxAndTransfer_LockupTranched_Integration_Concrete_Test
[PASS] test_WithdrawMaxAndTransfer_SameRecipient() (gas: 100436)
Traces:
[100436] WithdrawMaxAndTransfer_LockupTranched_Integration_Concrete_Test::test_WithdrawMaxAndTransfer_SameRecipient()
├─ [273] Defaults::WARP_26_PERCENT() [staticcall]
│ └─ ← [Return] 1714693400 [1.714e9]
├─ [0] VM::warp(1714693400 [1.714e9])
│ └─ ← [Return]
├─ [19862] LockupTranched::withdrawableAmountOf(1) [staticcall]
│ └─ ← [Return] 2600000000000000000000 [2.6e21]
├─ [0] VM::expectCall(DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xa9059cbb00000000000000000000000003e9b88f4b1406163ef9ec4875a52e1e55953ec100000000000000000000000000000000000000000000008cf23f909c0fa00000)
│ └─ ← [Return]
├─ [0] VM::expectEmit(LockupTranched: [0x923b5Ab3714FD343316aF5A5434582Fd16722523])
│ └─ ← [Return]
├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 2600000000000000000000 [2.6e21])
├─ [0] VM::expectEmit(LockupTranched: [0x923b5Ab3714FD343316aF5A5434582Fd16722523])
│ └─ ← [Return]
├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
├─ [0] VM::expectEmit(LockupTranched: [0x923b5Ab3714FD343316aF5A5434582Fd16722523])
│ └─ ← [Return]
├─ emit MetadataUpdate(_tokenId: 1)
├─ [49817] LockupTranched::withdrawMaxAndTransfer(1, Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1])
│ ├─ [12866] DAI::transfer(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], 2600000000000000000000 [2.6e21])
│ │ ├─ emit Transfer(from: LockupTranched: [0x923b5Ab3714FD343316aF5A5434582Fd16722523], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], value: 2600000000000000000000 [2.6e21])
│ │ └─ ← [Return] true
│ ├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 2600000000000000000000 [2.6e21])
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ ├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ └─ ← [Stop]
├─ [845] LockupTranched::getWithdrawnAmount(1) [staticcall]
│ └─ ← [Return] 2600000000000000000000 [2.6e21]
├─ [0] VM::assertEq(2600000000000000000000 [2.6e21], 2600000000000000000000 [2.6e21], "withdrawnAmount") [staticcall]
│ └─ ← [Return]
├─ [665] LockupTranched::getRecipient(1) [staticcall]
│ └─ ← [Return] Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1]
├─ [0] VM::assertEq(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], "recipient") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.83ms (294.71µs CPU time)
Ran 1 test for test/integration/concrete/lockup-linear/LockupLinear.t.sol:WithdrawMaxAndTransfer_LockupLinear_Integration_Concrete_Test
[PASS] test_WithdrawMaxAndTransfer_SameRecipient() (gas: 92432)
Traces:
[92432] WithdrawMaxAndTransfer_LockupLinear_Integration_Concrete_Test::test_WithdrawMaxAndTransfer_SameRecipient()
├─ [273] Defaults::WARP_26_PERCENT() [staticcall]
│ └─ ← [Return] 1714693400 [1.714e9]
├─ [0] VM::warp(1714693400 [1.714e9])
│ └─ ← [Return]
├─ [13202] LockupLinear::withdrawableAmountOf(1) [staticcall]
│ └─ ← [Return] 2600000000000000000000 [2.6e21]
├─ [0] VM::expectCall(DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xa9059cbb00000000000000000000000003e9b88f4b1406163ef9ec4875a52e1e55953ec100000000000000000000000000000000000000000000008cf23f909c0fa00000)
│ └─ ← [Return]
├─ [0] VM::expectEmit(LockupLinear: [0x4a6eD11Fa3612925cF0E72459893Ed6e33984E83])
│ └─ ← [Return]
├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 2600000000000000000000 [2.6e21])
├─ [0] VM::expectEmit(LockupLinear: [0x4a6eD11Fa3612925cF0E72459893Ed6e33984E83])
│ └─ ← [Return]
├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
├─ [0] VM::expectEmit(LockupLinear: [0x4a6eD11Fa3612925cF0E72459893Ed6e33984E83])
│ └─ ← [Return]
├─ emit MetadataUpdate(_tokenId: 1)
├─ [48479] LockupLinear::withdrawMaxAndTransfer(1, Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1])
│ ├─ [12866] DAI::transfer(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], 2600000000000000000000 [2.6e21])
│ │ ├─ emit Transfer(from: LockupLinear: [0x4a6eD11Fa3612925cF0E72459893Ed6e33984E83], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], value: 2600000000000000000000 [2.6e21])
│ │ └─ ← [Return] true
│ ├─ emit WithdrawFromLockupStream(streamId: 1, to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], asset: DAI: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], amount: 2600000000000000000000 [2.6e21])
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ ├─ emit Transfer(from: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], to: Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], tokenId: 1)
│ ├─ emit MetadataUpdate(_tokenId: 1)
│ └─ ← [Stop]
├─ [845] LockupLinear::getWithdrawnAmount(1) [staticcall]
│ └─ ← [Return] 2600000000000000000000 [2.6e21]
├─ [0] VM::assertEq(2600000000000000000000 [2.6e21], 2600000000000000000000 [2.6e21], "withdrawnAmount") [staticcall]
│ └─ ← [Return]
├─ [665] LockupLinear::getRecipient(1) [staticcall]
│ └─ ← [Return] Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1]
├─ [0] VM::assertEq(Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], Recipient: [0x03E9b88f4b1406163Ef9eC4875A52e1e55953eC1], "recipient") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.92ms (183.22µs CPU time)
Ran 3 test suites in 1.43s (25.90ms CPU time): 3 tests passed, 0 failed, 0 skipped (3 total tests)
Impact
It causes various impact
External Integrations and Dapps interacting with this smart contract might rely on the assumption that transfers only occur between distinct addresses. Self-transfers could interfere with these systems, causing them to malfunction or enter incorrect states and may even cause loss of funds depending on how external integration uses this function.
Self-transfers can disrupt the intended logic of the contract, leading to unexpected behaviors or states.
Self-transfers might be exploited to manipulate other streams' contract functions
Tools Used
Manual / Foundry
Recommendations
To prevent the self-transfer bug, implement a check in the withdrawMaxAndTransfer function to ensure that the newRecipient is different from the currentRecipient. If the newRecipient is the same as the currentRecipient, revert the transaction.
/// @inheritdoc ISablierV2Lockup
function withdrawMaxAndTransfer(
uint256 streamId,
address newRecipient
)
external
override
noDelegateCall
notNull(streamId)
{
// Check: the caller is the current recipient. This also checks that the NFT was not burned.
address currentRecipient = _ownerOf(streamId);
if (msg.sender != currentRecipient) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
// Skip the withdrawal if the withdrawable amount is zero.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (withdrawableAmount > 0) {
withdraw({ streamId: streamId, to: currentRecipient, amount: withdrawableAmount });
}
+ // Add check to prevent self-transfer
+ if (currentRecipient == newRecipient) {
+ revert Errors.SelfTransferNotAllowed(streamId, currentRecipient);
+ }
// Checks and Effects: transfer the NFT.
_transfer({ from: currentRecipient, to: newRecipient, tokenId: streamId });
}
Updates
Prize pool breakdown
Total prize
53,440 USDC
nSLOC
2,65520 USDC / LOC
45,000 USDC
3,100 USDC
5,340 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.