potential false positive for `calls-inside-a-loop`
- Dominant language
- Python
- Stars
- 6.4k
- Forks
- 1.1k
- PR merge metrics
- No merged PRs in 30d
Description
The example given https://github.com/crytic/slither/wiki/Detector-Documentation#calls-inside-a-loop is this:
```
contract CallsInLoop{
address[] destinations;
constructor(address[] newDestinations) public{
destinations = newDestinations;
}
function bad() external{
for (uint i=0; i < destinations.length; i++){
destinations[i].transfer(i);
}
}
}
```
The justification for `bad` being bad is that one of the `transfer` functions could error and then `bad` would never be able to be called again.
The finger is pointed at the loop, but the same problem occurs without any loop, e.g. this still may error irrecoverably for `0` thus bricking `1`:
```
contract CallsNotInLoop{
address[] destinations;
constructor(address[] newDestinations) public{
destinations = newDestinations;
}
function bad() external{
destinations[0].transfer(0);
destinations[1].transfer(1);
}
}
```
Removing the loop didn't remove the problem at all.
The only way that removing the loop "works" is if you remove loops by only ever calling one external function at a time.
However, only ever calling one external function at a time breaks atomic composition, which is more likely to introduce problems than the original loop was.
The real issue seems to me to be that `bad` is making external calls against immutable internal state rather than explicit runtime arguments.
consider:
```
contract CallsInLoopGood {
address[] destinations;
constructor(address[] newDestinations) public {
destinations = newDestinations;
}
function goodExplicit(address[] memory explicitDestinations) public {
for (uint i=0; i < explicitDestinations.length; i++){
explicitDestinations[i].transfer(i);
}
}
function good() external {
goodExplicit(destinations);
}
}
```
Here we still loop and `good` functions the same as `bad` did, even providing the same external interface, but if we ever find ourselves in a situation where some `transfer` call would produce an error, we can call `goodExplicit` directly without the offending destination and we restore the desired behaviour for all `transfer` calls that do not error, without losing atomic transfers.
This gives the power back to the caller to think about what they want to have happen here.
Maybe they wanted to transfer to destinations `X`, `Y` and `Z` but `Y` failed, luckily it failed atomically so they have the chance to decide for themselves whether or not to try again with only `X` and `Z`, or to walk away from the transaction.
Contributor guide
Assessment
This issue has not been assessed yet.