Strange results with require() in public view/pure-type functions

[wuya666]

Description

I cannot seem to find any official documentation on the correct behavior of a require() statement in a public view/pure-type function, but I think the current behavior is definitely not right.

Environment

• Compiler version: 0.4.24
• Framework/IDE (e.g. Truffle or Remix): Remix
• EVM execution environment / backend / blockchain client: Geth
• Operating system: Windows/Linux

Steps to Reproduce

There's a sample contract I have deployed on the mainnet

https://etherscan.io/address/0xd0b4e64c323186340ed2d8168ddae8f18677560e#code

pragma solidity ^0.4.24;

contract test {

    function sub1(uint256 _a, uint256 _b) public pure returns (uint256 result) {
        require(_a >= _b);
        return _a - _b;
    }

    function sub2(uint256 _a, uint256 _b) public pure returns (uint256 result) {
        require(_a >= _b, "_a cannot be less than _b");
        return _a - _b;
    }

}

So calling the sub1 and sub2 methods with Remix or web3 will produce strange results.

If I call sub1(3, 5) from Remix, it will return a result of 0, which is not exactly correct.

If I call sub1 from web3 with

test.methods.sub1(3,5).call().then(console.log).catch((err) => {console.log("Error: " + err);})

it will return

Error: Error: Couldn't decode uint256 from ABI: 0x

which is better than returning a result of 0, but still not sure it should be the correct error message.

Now the really problem is when I call sub2 with either Remix or web3 like

test.methods.sub2(3,5).call().then(console.log).catch((err) => {console.log("Error: " + err);})

It will in both cases (Remix and web3) return a non-error result of

3963877391197344453575983046348115674221700746820753546331534351508065746944

which is definitely NOT what I should get when doing a subtraction of 3 minus 5.

IMHO a require(false) in a public view/pure function should either generate an appropriate error message, or they should not be allowed in a public view/pure function.

[leonardoalt]

Hi @wuya666 , I just ran both cases on Remix, and the correct behavior happens which is to revert. Are your calls not reverting?

[wuya666]

it will say revert if you use the in-memory JavaScript VM, but if you use the real Ethereum blockchain via MetaMask injected web3 or infura.io or your own private net with Geth, it will return values instead of reverting.

[leonardoalt]

Ok, interesting. I just ran with MetaMask and it doesn't revert (reverts if pure is not used).

[wuya666]

yup, it will not revert if either view or pure is used. I guess since the public view or pure methods are called directly, there's no transaction and the state trie is not modified, so the VM just assumes there's nothing to revert anyway?

[leonardoalt]

That's what I'm guessing too. I'm now investigating what happens with the bytecode execution in that case (whether it just goes on from the revert point).

[leonardoalt]

I still think that a client's VM implementation should stop the execution when it encounters a revert though.

[wuya666]

exactly, or at least the client side (remix, web3) should be able to catch a "revert" message or something, at the very least not getting a 3963877391197344453575983046348115674221700746820753546331534351508065746944 result when doing 3 minus 5

[leonardoalt]

Since you were using Geth, could you open an issue on Geth's repo to see what they say?

[leonardoalt]

Just noticed that there was a discussion about this here: #3237

[wuya666]

it seems that issue #3237 is actually the opposite case of this. That issue is about NOT reverting the state in a pure function during an internal call, which will make all those internal pure SafeMath kind of libraries stop working.

The issue here that a direct external call to a view/pure function should be able to do revert and notice the calling party instead of returning values normally. Especially since now require(expression, string) is supported, that somehow may end up telling a Dapp front end that 3 minus 5 quals to 3963877391197344453575983046348115674221700746820753546331534351508065746944, which may even become a security vulnerability in the Dapp I guess.

[leonardoalt]

Yes, I meant a discussion on that being allowed or not, which you mentioned in the first post.

[kyriediculous]

You should be using assert instead if you are returning a uint (negative outcome would be type error and should be thus checked with assert and not revert/require).

This is mentioned in the docs.

Internally, Solidity performs a revert operation (instruction 0xfd) for a require-style exception and executes an invalid operation (instruction 0xfe) to throw an assert-style exception.

[leonardoalt]

I disagree with that. The code is filtering inputs for a safe operations, therefore it should use require, as the documentation states that is the case for filtering inputs. In any case, the only difference this would trigger would be the opcode change from REVERT to INVALID, and the clients/VMs should act accordingly in both cases.

[kyriediculous]

Trying to output a negative integer resulting from a numerical operation as a unsigned integer is an invalid operation. Not a require-style exception.

Require should be used only to check the condition of inputs, not the validity of the operation with those inputs. Eg. deadline > now or _tokenAmount > 0 You are misinterpreting the documentation here in my opinion, just look at the safemath library for reference.

The require function should be used to ensure valid conditions, such as inputs, or contract state variables are met, or to validate return values from calls to external contracts. If used properly, analysis tools can evaluate your contract to identify the conditions and function calls which will reach a failing assert. Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.

  /**
  * @dev Subtracts two numbers, throws on overflow (i.e. if subtrahend is greater than minuend).
  */
  function sub(uint256 a, uint256 b) internal pure returns (uint256) {
    assert(b <= a);
    return a - b;
  }

[leonardoalt]

Well, exactly, as the doc says, require should be used to ensure valid conditions, which in this case is the requirement for an operation that doesn't underflow. It's not an invalid operation by definition. Assertions should only be used to detect bugs. If the condition is true in that code, it doesn't mean that there is a bug, since the function is public and can be called with any values. In the end it's just input filtering. The SafeMath library uses require: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/math/SafeMath.sol.

[kyriediculous]

You're misinterpreting the documentation in my opinion. Checking conditions is context based. Trying to turn a uint into an int is overflow, it returns 0 to prevent the overflow Should be an assert as it should not happen.

Furthermore the safemath library has always used assert. Here is the correct version that should be used, I feel like: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/v1.12.0/contracts/math/SafeMath.sol

One other reason to assert is because it consumes gas and someone overflowing/underflowing is most often malicious intent.

Also it makes little sense to have pure functions that are public in my opinion , they are always called from another contract or internally.

[leonardoalt]

Trying to turn an uint into int does not necessarily cause an overflow, and it doesn't return 0. That's why it doesn't have to be an assert.

[leonardoalt]

I'm sorry but this discussion has moved away from the original problem discussed in this issue.

@kyriediculous please feel free to open another issue regarding any Solidity related discussion.

@wuya666 please upstream the issue to Geth, as it seems to be a problem with the client ignoring REVERT.

[kyriediculous]

I don't feel like this has moved away from the original problem, the essence of the problem is that require is wrongly used when it should be assert.

same like you would assert result = a - b assert( result < a ) because it overflows.

And no i was wrong indeed, it doesn't return 0 to prevent it. It returns an overflow. See below.

When doing literals in contract:

browser/test2.sol:3:24: TypeError: Return argument type int_const -2 is not implicitly convertible to expected type (type of first return variable) uint256.
                return 3-5;
                       ^-^

When doing parameters:

contract Sub {
        function sub(uint a, uint b)external pure returns(uint) {
                return a-b;
        }
}

decoded input	{ "uint256 a": "3", "uint256 b": "5" }
decoded output	{ "0": "uint256: 115792089237316195423570985008687907853269984665640564039457584007913129639934" }

[leonardoalt]

Exactly! The default behavior is to overflow. If in your context, you don't want overflows to be valid, you have to constrain it, and that's what require does here. You should use asserts only for conditions that are true in all cases that can reach that point. It's easy to see that in the following code, the condition _a >= b is not true for all cases, since the function is public and you can simply call it with, for example, 3 and 5.

function sub1(uint256 _a, uint256 _b) public pure returns (uint256 result) {
        require(_a >= _b);
        return _a - _b;
    }

And the essence of the problem in this issue is not that. The problem here is that the client ignores the REVERT opcode that is reached during the execution.

[leonardoalt]

So, I just tested the original problem with assert instead of revert, and as expected the result is exactly the same: the client ignores the INVALID since it's a public pure function and returns 0.

[kyriediculous]

I just tested it too and both require and assert work as expected.

pragma solidity ^0.4.24;

contract Sub {
  function sub1(uint256 a, uint256 b) public pure returns (uint256) {
    assert(b <= a);
    return a - b;
  }

  function sub2(uint256 a, uint256 b) public pure returns (uint256) {
    require(b  <= a);
    return a - b;
  }

  function sub3(uint256 a, uint256 b) public pure returns (uint256) {
    assert((a-b) < a);
    return a-b;
  }

}

VM Exception while processing transaction: invalid opcode VM Exception while processing transaction: revert VM Exception while processing transaction: invalid opcode

[leonardoalt]

As @wuya666 already pointed out, if you use Remix's Javascript VM it works fine, yes, but with MetaMask or directly with clients it doesn't.

[kyriediculous]

This is not javascript VM.

[leonardoalt]

Could you share the setup? The precise issue is when you call a pure/view function without an actual transaction.

[kyriediculous]

https://github.com/kyriediculous/solidity-playground

truffle test

tried on ganache and truffle testRPC.

[wuya666]

@kyriediculous I think you are confusing about different scenarios. According to the official docs here https://solidity.readthedocs.io/en/v0.4.24/control-structures.html#error-handling-assert-require-revert-and-exceptions

The assert function should only be used to test for internal errors, and to check invariants. The require function should be used to ensure valid conditions, such as inputs, or contract state variables are met, or to validate return values from calls to external contracts. If used properly, analysis tools can evaluate your contract to identify the conditions and function calls which will reach a failing assert. Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.

The example SafeMath library you referenced using asserts because it is an internal library, that means the calling party should be some other smart contract code which has already checked for the uint overflow conditions before calling the internal SafeMath library, and if the SafeMath library assert fails it means you need to fix your callling code in your contract.

My example here is a public function that can be called directly by Dapp front end outside the blockchain, thus require should be used to enforce valid conditions in the off-chain inputs.

And I have deployed a contract in the mainnet at the address 0xd0B4e64c323186340ed2D8168DdaE8f18677560E to demostrate the issue, like I have posted in the topic https://etherscan.io/address/0xd0b4e64c323186340ed2d8168ddae8f18677560e#code you may call the sub1 and sub2 methods with web3 or remix with MetaMask or infura.io to see how they receive a result of

3963877391197344453575983046348115674221700746820753546331534351508065746944

when calling things like sub2(1, 2), sub2(3, 5), etc.

[kyriediculous]

See my example. Only sub1 vs sub2 is up for discussion, sub3 should always be assert. in my opinion sub1 of my example is the correct way of doing this since it's an overflow error, not a contextual condition check. https://media.consensys.net/when-to-use-revert-assert-and-require-in-solidity-61fb2c0e5a57 I'll say again that having users call pure functions directly makes little sense to me, though.

This actually feels like a remix issue to me.

[leonardoalt]

I would like to move away from this discussion in this issue, so that we can focus on the actual problem. Regardless if assert or revert is used, there is a bug somewhere. I'm trying to nail down which client or framework might be problematic here. @wuya666 I've called the functions via Etherscan on the address you sent, and there it actually shows that it reverts. I've tested it with Metamask injected Web3 on Ropsten and Kovan and none revert

[kyriediculous]

Like i said, this feels like a Remix issue. Not a web3, solidity, ... issue. Because I didn't use remix and it worked perfectly.

I tried using Remix with the main net contracts and it didn't work.

I redeployed the contracts on rinkeby with an assert with remix and it didn't work.

Conclusion; issue with remix.

I haven't tested the main net contracts with just a web3 JS script yet. I could do that to provide more certainty.

[wuya666]

@kyriediculous the pure function is just an example, it can be a public view function. I just want to save gas deploying an example contract, as it costs me real money to deploy on mainnet you know. So yes issues like this can happen in real Dapps (and has happened in a real production Dapp I have deployed before, which is much more complex and NOT about integer overflows), not just the so-called "non-sensical" integer overflow/underflow public pure example I have posted here.

Also by the docs an assertion fail should never happen in real production contract code, if an assertion fail happens, you should fix your contract code, that means IMHO any method that can be called directly by external off-chain code/parties should NOT use assert to check input conditions, because you have no way to fix your contract code to avoid assertion failures as the input comes from outside the blockchain, which you have no control in your contract code.

@leonardoalt yup it seems somehow the etherscan "read contract" feature can somehow detect an VM execution error, but not Remix with MetaMask, nor the official web3js library. If I do for example

Web3 = require("web3");
web3 = new Web3("...")  // ... is some mainnet provider url, for example infura.io or local Geth full node
abi = ...  // ... is the abi json
contract = new web3.eth.Contract(abi);
contract.options.address = "0xd0B4e64c323186340ed2D8168DdaE8f18677560E"
contract.methods.sub2(3, 5).call().then(console.log)

It will return the absurd result of

3963877391197344453575983046348115674221700746820753546331534351508065746944

[kyriediculous]

Using ethers.js

call exception (address="0xd0b4e64c323186340ed2d8168ddae8f18677560e", method="sub1(uint256,uint256)", value=[3,5])

const ethers = require('ethers')
const provider = new ethers.providers.JsonRpcProvider('https://mainnet.infura.io/v3/apiKey')
const abi = [....]
const address = "0xd0b4e64c323186340ed2d8168ddae8f18677560e"
const sub = new ethers.Contract(address, abi , provider)
sub.sub1(3, 5).then(res => console.log(res)).catch(e => console.log(e.message))

[wuya666]

@kyriediculous it's definitely a web3 issue, or at least an official web3.js issue, not sure about the other web3 implementations like web3j or web3.py. That's when I encountered this strange issue when coding my Dapp with web3.js

Or maybe it's an Geth issue, since I have not tested with Parity full node yet...

On whether it's a solidity issue, I just think the offical docs are somewhat lacking in the specification of the behavior of require(), revert() etc. for direct external calls via public view/pure methods, especially after the recent implementation of require(expression, string)

[leonardoalt]

@wuya666 On Solidity's side the behavior is the same: require might lead to a revert, assert might lead to invalid, regardless the specifiers of the function.

I just ran with web3js and also got the weird value.

[wuya666]

@kyriediculous Have you tested calling sub2(3, 5) with ethers.js and see how it handles the returned string?

calling sub1(3, 5) with web3.js also returns a "Error: Couldn't decode uint256 from ABI: 0x" error, but calling sub2(3, 5) and it returns 3963877391197344453575983046348115674221700746820753546331534351508065746944

[wuya666]

@leonardoalt while they are not exactly the same, as for send transactions, require() only costs you a little gas (for the execution of the code up to the require failure), while assert() costs you all the gas you specified for the gas limit.

I guess that's related to why the official solidity doc specifies that for real production smart contract, assert() failure should never happen. The contract code itself needs to be fixed if an assert() failure can actually happen in your contract.

[leonardoalt]

Right, that makes sense.

Btw, I just ran with Ethers, and it does return an exception for sub1, and returns 3963877391197344453575983046348115674221700746820753546331534351508065746944 for sub2 ;)

Both Web3js and Ethers were connected to Infura.

[kyriediculous]

truffle-contract works as expected with ganache Remix with ganache hangs on calls

ethers works as expected with Rinkeby Remix with Rinkeby I get the 0's and weird value https://rinkeby.etherscan.io/address/0x3192474569cfa13354ad80406b73f416f1a9f0b4#readContract

eth-fiddle testRPC works https://ethfiddle.com/_kr9TW7MWm

web3 I keep getting that I only have 1 input parameter and the function requires two. (????)

Side-topic: I like how assert punishes malicious intent , for regular users it should be impossible to ever input params that yield an outcome <0 with good UX/UI.

[wuya666]

After some testing, it seems it's not Remix's issue, but rather it's the fault of the injected Web3 from MetaMask.

Remix's own JavaScript VM correctly returns the "VM revert" error, while the 0.2x.x web3.js library injected by MetaMask will return 0 for sub1(2,3) and the long int for sub2(2,3) as I have tested with the Chrome JS console with MetaMask.

I also tested the above Rinkeby contract with MetaMask's injected web3.js, and it returns 0 for sub1(2,3), sub2(2,3), sub3(2,3) and a long int for sub4(2,3).

When using web3.js 1.0.0beta, all of sub1(2,3), sub2(2,3), sub3(2,3) returns a "Couldn't decode uint256 from ABI: 0x" error, but still returns a long int for sub4(2,3) for the Rinkeby contract.

So @kyriediculous Could you confirm that ethers.js really works as expected for the sub4(2,3) method call for your Rinkeby contract, ie. not returning a long int result? Since it sounds strange that both web3.js and ethers.js return weird long int result on mainnet for the require(expression, string) statement, but on Rinkeby web3.js still returns weird long int result while ethers.js works as expected (I assume that means it reports an error)?

I have web3.js calling your Rinkeby contract no problem (except of course the weird return result of sub4), so I have no idea why you are getting the input parameter error with web3...

On the topic of assert, I like to punish malicious intent too, but then

1) the official solidity doc clearly specifies that it'd be a bug in your contract and that you should fix your contract code if assert failure can happen, nothing about fix your Dapp front end UI, so it's not really about what I like, I'd rather stick to the official doc as the accepted consensus for now.

Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.

2) there's no way to really tell whether it's really malicious intent, or if the one gets punished is indeed the malicious party.

3) I'm pretty sure that EVM did not intend the INVALID opcode to be used as a way to punish malicious intent, and if anything, recent events show that it has more likely become an easy way for malicious attackers to congest the Ethereum network.

[kyriediculous]

@wuya666 Terribly sorry I connected to ropsten instead of rinkeby on my ethers test so no wonder they all reverted

I just ran it again.

All work instead sub4 returning that number;

However the number doesn't change when changing input OR reasoning string... That's interesting.

additionally I created another function using if statement which has the exact same behaviour when using a reasoning string... returns the same number. Even when it's a different function.

Interesting....

function sub5(uint256 a, uint256 b) public pure returns (uint256) {
    if(b > a) revert("too large");
    return a-b;
  }

Here's the output of all of them:

1 call exception (address="0x523e0a5e5cb630392658a977fd1dc2a07c1cd81a", method="sub1(uint256,uint256)", value=[3,5])
5 3963877391197344453575983046348115674221700746820753546331534351508065746944
4 3963877391197344453575983046348115674221700746820753546331534351508065746944
2 call exception (address="0x523e0a5e5cb630392658a977fd1dc2a07c1cd81a", method="sub2(uint256,uint256)", value=[3,5])
3 call exception (address="0x523e0a5e5cb630392658a977fd1dc2a07c1cd81a", method="sub3(uint256,uint256)", value=[3,5])

And as you can see I used different inputs (And also different reasoning strings)

const ethers = require('ethers')
const provider = new ethers.providers.JsonRpcProvider('https://rinkeby.infura.io/v3/dcf1699d3526465b87dbeab3046bea2b')
const address = "0x523e0a5e5cb630392658a977fd1dc2a07c1cd81a"
const Sub = require('./build/contracts/Sub')
const sub = new ethers.Contract(address, Sub.abi, provider)

sub.sub1(3, 5).then(res => console.log("1 " + res)).catch(e => console.log("1 " + e.message))
sub.sub2(3, 5).then(res => console.log("2 " + res)).catch(e => console.log("2 " + e.message))
sub.sub3(3, 5).then(res => console.log("3 " + res)).catch(e => console.log("3 " + e.message))
sub.sub4(12, 29).then(r => console.log("4 " + r.toString(10))).catch(e => console.log(e.message))
sub.sub5(3, 3043).then(res => console.log("5 " + res)).catch(e => console.log('5 ' + e.message))

[kyriediculous]

Please check the reference and give Richard a thumbs up ! :)

[leonardoalt]

@kyriediculous thanks for reaching out to Ethers! @wuya666 web3 should be the same then, right?

[leonardoalt]

Closing as it's a JS issue.