openzeppelin-contracts
openzeppelin-contracts copied to clipboard
Behavior of ERC165Checker when less than 30k gas available
EIP-165 stipulate that supportsInterface
can use up to 30,000 gas.
But as you can see in openzeppelin implementation here, the call is executed without making sure 30,000 gas is indeed given to the call. Remember, the gas provided as part of the STATIC_CALL is just a maximum.
And because of EIP-150 behaviour, it is possible for supportsInterface
to get less gas than required for it to complete (and thus throw which is interpreted wrongly as non-implementation) while the rest of the transaction continue and complete.
I described the issue in more details here as the issue is also present in the example implementation described at EIP-165.
Various solution are presented here but the best option is EIP-1930 which also solve issue present in other use cases like meta-transactions.
Also find some test case regarding EIP-165 here
Hello @wighawag, thank you for reporting this!
How would you suggest to change the implementation? If, as you've mentioned, that function is called with less than 30k gas available, then the execution will error out with an 'out of gas' error, which is different from a regular revert. However, if there's indeed not enough gas, what could the contract do, other than erroring out early?
Hi @nventuro I mention various solution available today on EIP-1930 as well as on EIP-165 discussion
It can
- check for gas (via
gasleft()
) before the call but need to account for the gas required between the call togasleft()
and the actual call - check for gas afterward assuming supportsinterface is not quitting early based on the gas passed in,
But the best solution is to add functionality in the EVM for that : see EIP-1930
check for gas (via gasleft()) before the call but need to account for the gas required between the call to gasleft() and the actual call
What is the intent behind this? What should a contract do if it has less than 30k gas available to make the call?
check for gas afterward assuming supportsinterface is not quitting early based on the gas passed in,
30k will be sent if they are available: the only way for the implementer to quit early is if the transaction doesn't have enough gas, in which case the caller will revert with an out of gas error.
check for gas (via gasleft()) before the call but need to account for the gas required between the call to gasleft() and the actual call
What is the intent behind this? What should a contract do if it has less than 30k gas available to make the call?
_callERC165SupportsInterface
can be
function _callERC165SupportsInterface(address account, bytes4 interfaceId)
private
view
returns (bool success, bool result)
{
bytes memory encodedParams = abi.encodeWithSelector(_INTERFACE_ID_ERC165, interfaceId);
uint256 gasAvailable = gasleft() - E;
require(gasAvailable - gasAvailable / 64 >= 30000, "not enough gas provided")
// solhint-disable-next-line no-inline-assembly
assembly {
let encodedParams_data := add(0x20, encodedParams)
let encodedParams_size := mload(encodedParams)
let output := mload(0x40) // Find empty storage location using "free memory pointer"
mstore(output, 0x0)
success := staticcall(
30000, // 30k gas
account, // To addr
encodedParams_data,
encodedParams_size,
output,
0x20 // Outputs are 32 bytes long
)
result := mload(output) // Load the result
}
}
where E is the gas required for the operations between the call to gasleft()
and the actual call.
Unfortunately this computation will be dependent on gas pricing. And as such an overestimation is required.
For the intent, the idea is that if _callERC165SupportsInterface
can't ensure that supportsInterface
will receive 30,000 gas, it has to revert the call since it cannot be sure whether supportsInterface
throw because it did not received enough gas or simply because it does not implement supportsInterface
check for gas afterward assuming supportsinterface is not quitting early based on the gas passed in,
30k will be sent if they are available: the only way for the implementer to quit early is if the transaction doesn't have enough gas, in which case the caller will revert with an out of gas error.
While it is unlikely in the context of ERC-165 I wanted to make it clear that the "after-the-call" check does not work if the implementer of supportsInterface
is doing something like require(gasleft() > X
as in that case it could quit earlier because of a lack of gas, while not using all gas provided. If that happen the check gasleft() > 30,000 / 63
will not be sufficient since while less than 30,000 was given there could be more than 30,000/63 left.
Thanks for bringing this up @wighawag. Super interesting.
Unfortunately this computation will be dependent on gas pricing. And as such an overestimation is required.
To be clear, by "gas pricing" you mean the gas cost of each opcode, right?
I'm looking at the EIP and it seems rather clear that a reverting supportsInterface
should only be interpreted to mean "false" when querying about supporting ERC165 itself.
How to Detect if a Contract Implements ERC-165
- The source contract makes a
STATICCALL
to the destination address with input data:0x01ffc9a701ffc9a700000000000000000000000000000000000000000000000000000000
and gas 30,000. This corresponds tocontract.supportsInterface(0x01ffc9a7)
.- If the call fails or return false, the destination contract does not implement ERC-165.
- If the call returns true, a second call is made with input data
0x01ffc9a7ffffffff00000000000000000000000000000000000000000000000000000000
.- If the second call fails or returns true, the destination contract does not implement ERC-165.
- Otherwise it implements ERC-165.
How to Detect if a Contract Implements any Given Interface
- If you are not sure if the contract implements ERC-165, use the above procedure to confirm.
- If it does not implement ERC-165, then you will have to see what methods it uses the old-fashioned way.
- If it implements ERC-165 then just call
supportsInterface(interfaceID)
to determine if it implements an interface you can use.
Note how the second list doesn't mention anything about a failing call. This impies that reverts must be propagated.
So there are two bugs in our implementation:
-
ERC165Checker._supportsERC165
is affected by the gas-related bug reported in this issue. -
ERC165Checker._supportsInterface
is not propagating reverts like it should be.
The second bug is easy to fix. The first one is the tricky one.
the "after-the-call" check does not work if the implementer of
supportsInterface
is doing something likerequire(gasleft() > X)
@wighawag In this case it is the implementer who is buggy. Because of the following line from the EIP:
If the call fails or return false, the destination contract does not implement ERC-165.
This implies that if the implementer indeed does require(gasleft() > X)
it should be interpreted as not implementing ERC165. (This definitely deserves special mention in the EIP.)
With this in mind, do you think an after-the-call check would work? I will give this a bit more thought and get back.
Thanks for reporting this @wighawag, I had forgotten about the 63/64ths rule.
In the worst case, a contract needs 30k gas to execute its function, and is provided only 29,999. This means that the caller will end up with 476 gas (29999 / 63
) after the (failed) call. I don't think 476 gas is enough for a contract to perform any significant action (and this is the absolute worst case), but it is enough for a view
function to return, which may be used to trick whomever is calling (maybe a dApp's UI)? We should therefore definitely fix this.
Pasting here @wighawag's suggestion from https://github.com/ethereum/EIPs/pull/881#issuecomment-491677748:
check for gasleft() after the call
// execute STATIC_CALL with 30000 gas require(gasleft() > 30000/63, "not enough gas left");
This works because if the call throw because of not enough gas, the amount of gas left will be lower than 30000/63. But this also require for
supportsInterface
to not have code likerequire(gasleft() > X)
since this in that case, the gas left after the call would be bigger than what it would be if all the gas was used This does not depend on gas pricing except for the 1/64 behavior of EIP-150
Given the considerations above, I think this is the best course of action.
@frangio is the idea behind that check to not require the caller tx to have over 30k gas? Shouldn't we also only revert if the call failed? i.e. if the call failed and it possibly run out of gas (there's less than 30k/63 remaining), then we revert.
i.e. if the call failed and it possibly run out of gas (there's less than 30k/63 remaining), then we revert.
Yes. Why "possibly" though?
A call may return with less than that value and not have reverted, or It may have reverted for a different reason.
Technically we can assume that the implementer will use at most 30000 gas. So we could send like 30001 (or a similarly small value), and then we can be sure that if there is less than 30k/64 we can assume that it was an out of gas error.
I really am not sure about all these numbers though. I need to sit down and think them through.
...but the whole point of this is what happens when less than 30k are sent? And a post-check will not know how much was actually sent.
@frangio
To be clear, by "gas pricing" you mean the gas cost of each opcode, right?
yes that's what I meant
Note how the second list doesn't mention anything about a failing call. This impies that reverts must be propagated.
That's true, but this is not very clear since it does not mention the 30,000 rules neither here
In this case it is the implementer who is buggy. Because of the following line from the EIP:
Possibly but the require(gasleft() >X)
could also come from a call that supportsInterface
make independent of it. So I don't think we can consider that buggy in all case, unless ERC-165 change to precise such forbidden behaviour
Also it is worth noting that an assert(gasleft() > X)
is fine since all gas is used
@nventuro
I don't think 476 gas is enough for a contract to perform any significant action (and this is the absolute worst case)
The thing is that the caller might not have to do anything. I actually discovered the bug while I was investigating the use of 165 for token receiver in #1155
the context is that a contract that want to act on token reception must implement onERC1155Received
and that method can throw to reject the reception of tokens
The logic for the caller could be (like in the case of an unsafeTransfer
method :
- check existence of interface X
- if it exits, it means the receiver accept a call to
onERC1155Received
- call
onERC1155Received
- if it throw, revert the transfer
- call
- if the interface X do not exit, continue the transfer
The last step could be doing nothing else, just returning, this is where it does not need much gas.
That's true, but this is not very clear since it does not mention the 30,000 rules neither here
That requirement is mentioned in the general description for `supportsInterface:
This function must return a bool and use at most 30,000 gas.
What I meant is that the 2nd list is less clear, it could have at least said "call supportsInterface(interfaceID) with at least 30,000 gas" Especially in comparison to the first list
I agree though that your interpretation is correct.
This is still an issue but I don't believe it would be right to encode the 63/64ths rule in the Solidity code. I think this is just a limitation that users of this EIP have to consider. It needs to be documented.
if this issued is solved then we can close the issue.