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

[wolflo]

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 withdrawal method for sending funds to untrusted addresses).

// 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.:

(bool success, bytes memory data) = address(unseatedKing).call{value: unseatedBid}("");

[cameel]

Right, this does sound like a sensible optimization. I suppose it was done this way because we generally try to make the code generation straightforward and rely on the optimizer to remove unused stuff instead. In this case it looks like this is not being optimized out even though memory goes unused. This is something that the upcoming RedundantStoreEliminator (#11352) might be able to handle in the Yul optimizer but I think this is simple enough to change in the codegen that it might be worth doing there. For example in the old codegen the change would probably be limited to just this part:

solidity/libsolidity/codegen/ExpressionCompiler.cpp

Lines 2725 to 2761 in defc74c

	else if (!returnTypes.empty())
	{
	utils().fetchFreeMemoryPointer();
	// Stack: return_data_start
	// The old decoder did not allocate any memory (i.e. did not touch the free
	// memory pointer), but kept references to the return data for
	// (statically-sized) arrays
	bool needToUpdateFreeMemoryPtr = false;
	if (dynamicReturnSize || m_context.useABICoderV2())
	needToUpdateFreeMemoryPtr = true;
	else
	for (auto const& retType: returnTypes)
	if (dynamic_cast<ReferenceType const*>(retType))
	needToUpdateFreeMemoryPtr = true;
	// Stack: return_data_start
	if (dynamicReturnSize)
	{
	solAssert(haveReturndatacopy, "");
	m_context.appendInlineAssembly("{ returndatacopy(return_data_start, 0, returndatasize()) }", {"return_data_start"});
	}
	else
	solAssert(retSize > 0, "");
	// Always use the actual return length, and not our calculated expected length, if returndatacopy is supported.
	// This ensures it can catch badly formatted input from external calls.
	m_context << (haveReturndatacopy ? evmasm::AssemblyItem(Instruction::RETURNDATASIZE) : u256(retSize));
	// Stack: return_data_start return_data_size
	if (needToUpdateFreeMemoryPtr)
	m_context.appendInlineAssembly(R"({
	// round size to the next multiple of 32
	let newMem := add(start, and(add(size, 0x1f), not(0x1f)))
	mstore(0x40, newMem)
	})", {"start", "size"});
	utils().abiDecode(returnTypes, true);
	}

At least as long as we're talking about the case where the return data goes completely unused (which would cover the low-level calls). Handling the situation where it's only partially used (i.e. you ignore some but not all return values) in a high-level call would be more complex and might require changes in the ABI decoder code.

[chriseth]

Related to @hrkrshnn 's focus task of tracking memory references. I'm not sure we should really even further complicate the call code.

[wolflo]

Understandable that complicating the code generation here is a major concern.

I would argue that this is a subtle security issue, and is at least worth adding to the docs (happy to do that if that's the best way forward). Being able to force a contract to copy unlimited data into memory after any external call is potentially a serious problem for many contracts.

[frangio]

@chriseth Is there a link where we could track the work on tracking memory references? I was just looking to propose a related optimization.

[cameel]

I don't think we have one overarching issue for that. There's a bunch of issues about tracking memory stores and removing redundant ones (#10690, #10755, #6727, #12211), there are also a few ongoing PRs (#12272, #11352, #11192).

You can post in one that looks relevant but if it's a completely new thing I think it would be fine to just open a new issue specifically for that optimization.

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[frangio]

This should be fixed.

[ekpyron]

The really nice, though quite complex, way to fix this would be to treat returndata as another data location and only actually copy from there to memory on a conversion.
Short of that, as mentioned above, memory lifetime tracking should potentially make it possible to remove the copy automatically, but that may not be easy.

I'll label the issue, s.t. it remains open in any case.