Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of empty array check in the `Sablier::Batch` function.

oxenzo

Description:

The batch function in the contract allows users to execute multiple function calls in one transaction. However it lacks a check for empty arrays, which can lead to unintended behaviour. If an empty array is passed as an argument, the function will attempt to iterate over it, resulting in no operations being performed and potential confusion for users.

https://github.com/Cyfrin/2024-10-sablier/blob/8a2eac7a916080f2022527408b004578b21c51d0/src/abstracts/Batch.sol#L16

Impact:

While passing an empty array does not directly risk funds, it can disrupt the intended functionality of the batching mechanism. This may lead to user frustration, wasted gas fees, and decreased trust in the contract’s reliability, especially in scenarios where automated processes depend on this function.

Proof of Concept:

Place this test in the batch.t.sol file.

function test_Batch_WithZeroaddress() external {
bytes[] memory calls = new bytes[]();
flow.batch(calls);
}

The function passes unexpectedly as seen below:

[6337] Batch_Integration_Concrete_Test::test_Batch_WithZeroaddress()
├─ [634] Flow::batch([])
│ └─ ← [Stop]
└─ ← [Stop]

Recommended Mitigation:

Would recommend to add a empty arrays check as seen below:

function batch(bytes[] calldata calls) external {
uint256 count = calls.length;
+ if (count == 0) revert Errors.EmptyBatch();
​
for (uint256 i = 0; i < count; ++i) {
(bool success, bytes memory result) = address(this).delegatecall(calls[i]);
if (!success) {
revert Errors.BatchError(result);
}
}
}
​

When the same function is tested after the implemented fix this what outputs in the logs:

[9354] Batch_Integration_Concrete_Test::test_Batch_WithZeroaddress()
├─ [0] VM::expectRevert(custom error 0xc31eb0e0: c2e5347d00000000000000000000000000000000000000000000000000000000)
│ └─ ← [Return]
├─ [654] Flow::batch([])
│ └─ ← [Revert] EmptyBatch()
└─ ← [Stop]
​

Meaning that the fix is effective.

Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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