argotorg / argotorg/solidity

External calls should not generate RETURNDATACOPY if the return data is not used

Open
#12,306 8 comments 8 reactions 0 assignees View on GitHub
medium effort medium impact must have eventually optimizer
Dominant language
C++
Stars
25.7k
Forks
6.2k
Avg merge
1d 11h
Merged PRs (30d)
21

Description

## Description

Solidity automatically copies all return data into memory after any external call, including low-level calls for which the return data is never captured or used. To my knowledge, the only way to prevent this is to make the call from an inline assembly block.

This is somewhat counterintuitive and makes it difficult to reason about the resource consumption of an untrusted external call. Here is an example of a subtle vulnerability resulting from this behavior (acknowledging that this violates the [suggested](https://docs.soliditylang.org/en/v0.8.10/common-patterns.html#withdrawal-from-contracts) withdrawal method for sending funds to untrusted addresses).

```solidity
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.0;

contract KingOfTheHill {
address public king;
uint256 public topBid;

function bid() external payable {
require(msg.value > topBid);
address unseatedKing = king;
uint256 unseatedBid = topBid;
king = msg.sender;
topBid = msg.value;
// Return the previous king's bid amount.
// Common to assume that 1/64 of gasleft() before the
// call will be available to complete execution.
address(unseatedKing).call{value: unseatedBid}("");
}
}

contract EvilKing {
KingOfTheHill private hill;
constructor(KingOfTheHill _hill) { hill = _hill; }
function doBid() external payable {
hill.bid{value: msg.value}();
}

// Force calling contract to consume remaining gas in RETURNDATACOPY
// by returning as much 0 data as possible without running out of gas.
fallback() external payable override {
// approximate solution to Cmem for new_mem_size_words
uint256 rsize = sqrt(gasleft() / 2 * 512);
assembly {
return(0x0, mul(rsize, 0x20))
}
}

function sqrt(uint x) private returns (uint y) {
uint z = (x + 1) / 2;
y = x;
while (z < y) {
y = z;
z = (x / z + z) / 2;
}
}
}
```

It is common to assume that the external call made in `KingOfTheHill.bid()` will always have some gas remaining after the call to complete execution. In theory, the `unseatedKing` address receiving this call will be able to consume a maximum of `63/64 * G` gas, where G is the gas remaining at the callsite. This may lead to the assumption that there is always some amount of gas that a bidder can provide to the `bid()` call such that the remaining `1/64 * G` gas will be enough to finish execution without exhausting the gas.

But, the call recipient can force the caller to consume gas *after* the call by returning a large amount of data.

As seen above, if the `unseatedKing` is a contract that returns as much 0 data as possible without throwing an `OUT_OF_GAS` exception, there is no amount of gas that a bidder can provide to `bid()` such that execution does not run out of gas after the call to the `unseatedKing`. All of the data that the `unseatedKing` contract returns will be copied into memory after the call, and `RETURNDATACOPY` is more expensive per byte of data than `RETURN`. The `unseatedKing` gets 63/64 of remaining gas `G` to return as much data as possible, and the `bid()` function can never copy that much data into memory without exhausting the remaining `1/64 * G` gas.

## Expected behavior
Do not `RETURNDATACOPY` after a low-level external call unless the return data is captured or used, e.g.:
```solidity
(bool success, bytes memory data) = address(unseatedKing).call{value: unseatedBid}("");
```

Contributor guide

Open the contributing guide

Research direction

Start by tracing how the compiler lowers low-level external calls whose return data is ignored, using the KingOfTheHill example in the issue. Compare the generated behavior with a call whose return data is captured, and verify that ignored return data is not copied while captured data remains available. Done means the described post-call gas exhaustion is avoided without changing calls that use their return data.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, solidity
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.