Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behavior of ERC165Checker when less than 30k gas available #1750

Open
wighawag opened this issue May 13, 2019 · 19 comments
Open

Behavior of ERC165Checker when less than 30k gas available #1750

wighawag opened this issue May 13, 2019 · 19 comments
Labels
bug contracts Smart contract code.

Comments

@wighawag
Copy link

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

@nventuro
Copy link
Contributor

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?

@wighawag
Copy link
Author

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 to gasleft() 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

@nventuro
Copy link
Contributor

nventuro commented May 13, 2019

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.

@wighawag
Copy link
Author

wighawag commented May 13, 2019

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.

@frangio
Copy link
Contributor

frangio commented May 14, 2019

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?

@frangio
Copy link
Contributor

frangio commented May 14, 2019

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

  1. The source contract makes a STATICCALL to the destination address with input data: 0x01ffc9a701ffc9a700000000000000000000000000000000000000000000000000000000 and gas 30,000. This corresponds to contract.supportsInterface(0x01ffc9a7).
  2. If the call fails or return false, the destination contract does not implement ERC-165.
  3. If the call returns true, a second call is made with input data 0x01ffc9a7ffffffff00000000000000000000000000000000000000000000000000000000.
  4. If the second call fails or returns true, the destination contract does not implement ERC-165.
  5. Otherwise it implements ERC-165.

How to Detect if a Contract Implements any Given Interface

  1. If you are not sure if the contract implements ERC-165, use the above procedure to confirm.
  2. If it does not implement ERC-165, then you will have to see what methods it uses the old-fashioned way.
  3. 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:

  1. ERC165Checker._supportsERC165 is affected by the gas-related bug reported in this issue.
  2. ERC165Checker._supportsInterface is not propagating reverts like it should be.

@frangio
Copy link
Contributor

frangio commented May 14, 2019

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 like require(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.

@nventuro
Copy link
Contributor

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.

@nventuro nventuro added bug contracts Smart contract code. labels May 14, 2019
@frangio
Copy link
Contributor

frangio commented May 14, 2019

Pasting here @wighawag's suggestion from ethereum/EIPs#881 (comment):

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 like require(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.

@nventuro
Copy link
Contributor

@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.

@frangio
Copy link
Contributor

frangio commented May 14, 2019

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?

@nventuro
Copy link
Contributor

A call may return with less than that value and not have reverted, or It may have reverted for a different reason.

@frangio
Copy link
Contributor

frangio commented May 14, 2019

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.

@nventuro
Copy link
Contributor

nventuro commented May 14, 2019

...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 frangio changed the title ERC165Checker is not checking for gas : supportsInterface can throw as a result even if it would have returned true otherwise Behavior of ERC165Checker when less than 30k gas available May 14, 2019
@wighawag
Copy link
Author

@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
  • 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.

@frangio
Copy link
Contributor

frangio commented May 15, 2019

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.

@wighawag
Copy link
Author

wighawag commented May 15, 2019

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.

@nventuro nventuro added this to the v2.4 milestone May 15, 2019
@frangio frangio removed this from the v2.4 milestone Sep 16, 2022
@frangio
Copy link
Contributor

frangio commented Oct 8, 2022

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.

@HariharPadhi1412
Copy link

if this issued is solved then we can close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contracts Smart contract code.
Projects
None yet
Development

No branches or pull requests

4 participants