raise ValidationError if ether is sent to non payable functions #1017

ankit-maverick · 2018-08-24T05:34:15Z

What was wrong?

Related to Issue #902

How was it fixed?

Raised ValidationError in call, transact, estimateGas, buildTransaction if ether is sent to functions that are not payable.

TODOs:

Get the approach and code reviewed by @carver
Write tests

Cute Animal Picture

voith · 2018-08-24T11:57:06Z

web3/contract.py

@@ -432,6 +433,15 @@ def estimateGas(self, transaction=None):
                    "Please ensure that this contract instance has an address."
                )

+        if 'value' in estimate_transaction:


instead of duplicating this logic in multiple places, I think all of this code can go in the validation middleware.

Agreed. Looks like this can be a def validate_payable(transaction, abi) to consolidate the logic. Other than that, the approach is fine 👍

yeah, a helper function would do.
The problem with the middleware approach is that the abi will not be available there.

@voith I was wondering the same when I looked at the validation code in middleware since abi is not available there.

@carver Yeah, I will consolidate it.

ankit-maverick · 2018-08-25T03:51:46Z

@voith @carver I have consolidated the logic as discussed and decided to keep def validate_payable(transaction, abi) in contract.py. Let me know if you think there's a better place to keep it.

I was wondering if tests are needed for this change and if yes, I need some direction on how I should go about them. I tried to look up tests inside tests/core/contracts/ for the various ValueErrors being raised in call, transact, estimateGas, buildTransaction in contract.py but could not find any tests corresponding to them.

ankit-maverick · 2018-08-27T16:35:50Z

@carver @voith ^Need your inputs on the above comment.

pipermerriam · 2018-08-27T16:53:34Z

@ankit-maverick yes, we'll want to have tests added for this. These should probably be done with a new contract as to not conflate any of the existing testing contracts. Something simple like

contract PayableTester {
  bool public wasCalled;

  function doPayableCall() public payable {
    wasCalled = true;
  }
  function doNoValueCall() public {
    wasCalled = true;
  }
}

For testing those, you can do something simple like:

pre check that wasCalled == False
try to transact with/without value
verify exception is raised (if one is expected)
verify wasCalled is the expected value.

These tests should be done for all of call/transact/estimateGas/buildTransaction and clearly some of the assertions in the list above don't make sense for anything other than transact.

pipermerriam · 2018-09-03T18:08:48Z

@ankit-maverick let me know if you need help getting the tests green.

ankit-maverick · 2018-09-06T12:46:32Z

@pipermerriam Sure. I will finish this off this weekend and ask here when I need help.

ankit-maverick · 2018-09-06T14:36:19Z

@pipermerriam One issue I faced last weekend with these tests is understanding the difference between 2 runs of the same tests [<lambda>-func_first] that got PASSED and [<lambda>-func_last] that got FAILED.

tests/core/contracts/test_contract_buildTransaction.py::test_build_transaction_paying_to_nonpayable_function[<lambda>-func_first] PASSED                                                                 [  5%]
tests/core/contracts/test_contract_buildTransaction.py::test_build_transaction_paying_to_nonpayable_function[<lambda>-func_last] FAILED                                                                  [  7%]
.
.
.
tests/core/contracts/test_contract_buildTransaction.py::test_build_transaction_paying_to_nonpayable_function[address_conversion_func1-func_first] PASSED                                                 [ 55%]
tests/core/contracts/test_contract_buildTransaction.py::test_build_transaction_paying_to_nonpayable_function[address_conversion_func1-func_last] FAILED                                                  [ 57%]

What's difference between both of them?

Edit : Grammar

carver · 2018-09-06T20:55:24Z

There is an old API style for interacting with contracts that looks like:

contract.buildTransaction(...transaction_params...).doNoValueCall()

(ie~ function last)

versus the new style of:

contract.functions.doNoValueCall().buildTransaction(...transaction_params...)

(ie~ function first)

The test failures imply that the changes work with the new style, but not the old style, API.

If you're curious, here is how the tests are set up:

web3.py/tests/core/contracts/conftest.py

Lines 547 to 553 in 9800597

    
           if api_style == 'func_first': 
        
               function = contract.functions[contract_function] 
        
               result = getattr(function(*func_args, **func_kwargs), api_call_desig)(tx_params) 
        
           elif api_style == 'func_last': 
        
               api_call_cls = getattr(contract, api_call_desig) 
        
               with pytest.deprecated_call(): 
        
                   result = getattr(api_call_cls(tx_params), contract_function)(*func_args, **func_kwargs)

ankit-maverick · 2018-09-09T13:59:34Z

@carver @pipermerriam I have finished tests for buildTransaction, call, estimateGas, transact but am finding it tough to understand the error logs to see why the old API style (func_last) for interacting with contracts is failing when I pass tx_params={'value': 1} to any of these function calls. Need your help on this.

Edit : It doesn't fail in the way it is currently failing when I pass any other parameter like gasPrice or don't pass any parameter at all. The error stacktrace flew through evm and eth-tester but I couldn't make much sense of it and find the root cause of the issue.

carver · 2018-09-10T20:26:35Z

tests/core/contracts/conftest.py

+
+  function doPayableCall() public payable {
+    wasCalled = true;
+  }


It looks like we're never really testing doPayableCall here. That's maybe fine (since it should be covered by other tests), but let's remove it from the contract and ABI, then.

yeah, I will remove it.

carver · 2018-09-10T20:36:27Z

tests/core/contracts/test_contract_estimateGas.py

+        payable_tester_contract,
+        estimateGas):
+    with pytest.raises(ValidationError):
+        estimateGas(contract=payable_tester_contract,


I would focus on why this is failing. Many of the other functions will fail if this fails, because calls like buildTransaction will call estimateGas under the hood, if no gas limit is supplied (which is common).

Can you reproduce outside of testing, with payable_tester_contract.estimateGas(dict(value=1)).doNoValueCall()?

I could reproduce it. Will dig deeper into it.

>>> contract_instance.estimateGas(dict(value=1)).doNoValueCall() API Exception: {'type': 'TransactionFailed', 'args': (), 'message': ''} Traceback (most recent call last): File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/jsonrpc/manager.py", line 112, in _get_responses File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/server.py", line 34, in inner File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/rpc.py", line 138, in eth_estimateGas File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/client/client.py", line 253, in estimate_gas File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/client/client.py", line 263, in send_transaction File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/client/client.py", line 58, in inner File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/client/utils.py", line 104, in inner File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/testrpc/client/client.py", line 225, in _send_transaction File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/ethereum/tester.py", line 338, in send File "/Users/ankit/web3.py/venv/lib/python3.7/site-packages/ethereum/tester.py", line 296, in _send ethereum.tester.TransactionFailed 127.0.0.1 - - [13/Sep/2018 08:08:42] "POST / HTTP/1.1" 200 147 Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ankit/web3.py/web3/contract.py", line 1506, in estimate_gas_for_function gas_estimate = web3.eth.estimateGas(estimate_transaction) File "/Users/ankit/web3.py/web3/eth.py", line 304, in estimateGas [transaction], File "/Users/ankit/web3.py/web3/manager.py", line 112, in request_blocking raise ValueError(response["error"]) ValueError: {'code': -32000, 'message': 'Server error', 'data': {'type': 'TransactionFailed', 'args': [], 'message': ''}}

voith · 2018-09-13T15:25:21Z

@ankit-maverick So here's what's happening:
The old API looked something like this:

contract.transact({'value': 1}).doNoValueCall()

Notice here that the function doNoValueCall() is called after transact().
Your validate_payable expects to get abi of the specific function that is being called.

Now for the old API you've added validation in thetransact method. However, since the function is called after calling transact, self.abi contains the entire contract abi (list) and hence your check
if "payable" in abi and not abi["payable"] fails.

You should probably add the check inside Transactor where you will get the function name, from which you can extract the correct abi:

web3.py/web3/contract.py

Line 591 in d436b0a

def __getattr__(self, function_name):

Do the same for other methods like estimateGas, call...

pipermerriam · 2018-09-13T15:36:05Z

tests/core/contracts/conftest.py

-def FALLBACK_FUNCTION_CONTRACT(FALLBACK_FUNCTION_CODE,
-                               FALLBACK_FUNCTION_RUNTIME,
-                               FALLBACK_FUNCTION_ABI):
+def PAYABLE_TESTER_CONTRACT(PAYABLE_TESTER_CODE,


cc @njgheorghita I was looking at this stuff and it might be a good test-bed for seeing how py-ethpm and maybe pytest-ethereum could clean up these test fixtures.

ankit-maverick · 2018-09-15T17:29:56Z

@voith Thanks a ton. I have moved validate_payable inside Transactor, Caller for old API methods in the latest commit and the old API tests are passing.

ankit-maverick · 2018-09-15T19:18:51Z

@carver @pipermerriam @voith Need your final review on this PR.

voith · 2018-09-15T22:12:02Z

web3/contract.py

+
+def get_function_abi_from_contract_abi(contract_abi, function_name):
+    for abi in contract_abi:
+        if abi['name'] == function_name:


A contract can have multiple functions with the same name but different signatures.
eg.

contract FunctionOverloadingContract { function someMethod(int i) public { // do something } function someMethod(string s) public { // do something } }

get_function_abi_from_contract_abi might return the wrong abi in such a case.

Have a look find_matching_fn_abi to understand more.
This code makes me think that the validation should probably be moved to methods like call_contract_function, transact_with_contract_function, build_transaction_for_function, estimate_gas_for_function . I have not done a complete analysis, but just throwing an idea.

@voith This idea makes more sense than the approach I have taken. I could get rid of get_function_abi_from_contract_abi and better use find_matching_fn_abi & check validate_payable inside lower level base functions call_contract_function, transact_with_contract_function, build_transaction_for_function, estimate_gas_for_function which are being called from both new and old style APIs.

ankit-maverick · 2018-09-16T11:59:52Z

@carver @pipermerriam @voith Please review. If all looks good, I will squash the commits before we merge this.

carver · 2018-09-17T18:24:04Z

web3/contract.py

+    if fn_abi is None:
+        fn_abi = find_matching_fn_abi(contract_abi, function_name, args, kwargs)
+
+    validate_payable(transaction, fn_abi)


It looks like all four of these copy/pasted chunks come just before a prepare_transaction(). Can the lines move inside prepare_transaction(), so the repeats can be deleted?

Yeah, don't know how I missed it. Pushing the change now.

carver · 2018-09-18T17:39:11Z

Great, I think this is ready for your squash and we can get it merged @ankit-maverick !

ankit-maverick · 2018-09-19T05:44:00Z

@carver @pipermerriam Ready to go in.

ankit-maverick · 2018-09-20T19:53:45Z

@carver @voith @pipermerriam Thanks a lot. This was my first contribution to anything crypto and I learned a lot of concepts while working through this PR(although I assumed it to be a very straightforward PR in the beginning).

carver · 2018-09-20T20:13:58Z

Thanks for the contribution @ankit-maverick !

ccolorado · 2018-10-15T02:12:18Z

Will this make it to the next release (4.7.2)?
I don't want to think on how less miserable my life would have been, if this was included on 4.7.1

carver · 2018-10-15T20:37:45Z

Ah, this went into the master branch which is on v5, it would need to be backported and merged into v4 to go into 4.7.2.

voith reviewed Aug 24, 2018

View reviewed changes

carver reviewed Sep 10, 2018

View reviewed changes

pipermerriam reviewed Sep 13, 2018

View reviewed changes

ankit-maverick force-pushed the fix-902 branch from 912b9f1 to deb5806 Compare September 15, 2018 18:54

voith reviewed Sep 15, 2018

View reviewed changes

carver reviewed Sep 17, 2018

View reviewed changes

ankit-maverick force-pushed the fix-902 branch from eba38fb to 7d83c0e Compare September 19, 2018 05:02

carver force-pushed the fix-902 branch from 36339f8 to d3f029e Compare September 19, 2018 19:29

raise ValidationError on sending ether to non-payable functions

ef616c5

carver force-pushed the fix-902 branch from d3f029e to ef616c5 Compare September 19, 2018 20:19

carver merged commit 5358a91 into ethereum:master Sep 19, 2018

gitcoinbot mentioned this pull request Sep 20, 2018

raise validation error if trying to send ether to a contract function with payable=False #902

Closed

carver mentioned this pull request Oct 18, 2018

prevent sending ether to non-payable functions #1115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raise ValidationError if ether is sent to non payable functions #1017

raise ValidationError if ether is sent to non payable functions #1017

ankit-maverick commented Aug 24, 2018 •

edited

Loading

voith Aug 24, 2018

carver Aug 24, 2018

voith Aug 24, 2018

ankit-maverick Aug 25, 2018 •

edited

Loading

ankit-maverick commented Aug 25, 2018 •

edited

Loading

ankit-maverick commented Aug 27, 2018

pipermerriam commented Aug 27, 2018 •

edited by carver

Loading

pipermerriam commented Sep 3, 2018

ankit-maverick commented Sep 6, 2018

ankit-maverick commented Sep 6, 2018 •

edited

Loading

carver commented Sep 6, 2018 •

edited

Loading

ankit-maverick commented Sep 9, 2018 •

edited

Loading

carver Sep 10, 2018

ankit-maverick Sep 13, 2018

carver Sep 10, 2018

ankit-maverick Sep 13, 2018

voith commented Sep 13, 2018 •

edited

Loading

pipermerriam Sep 13, 2018

ankit-maverick commented Sep 15, 2018

ankit-maverick commented Sep 15, 2018

voith Sep 15, 2018 •

edited

Loading

ankit-maverick Sep 16, 2018

ankit-maverick commented Sep 16, 2018

carver Sep 17, 2018

ankit-maverick Sep 18, 2018 •

edited

Loading

carver commented Sep 18, 2018

ankit-maverick commented Sep 19, 2018

ankit-maverick commented Sep 20, 2018

carver commented Sep 20, 2018

ccolorado commented Oct 15, 2018

carver commented Oct 15, 2018

raise ValidationError if ether is sent to non payable functions #1017

raise ValidationError if ether is sent to non payable functions #1017

Conversation

ankit-maverick commented Aug 24, 2018 • edited Loading

What was wrong?

How was it fixed?

TODOs:

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankit-maverick Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

ankit-maverick commented Aug 25, 2018 • edited Loading

ankit-maverick commented Aug 27, 2018

pipermerriam commented Aug 27, 2018 • edited by carver Loading

pipermerriam commented Sep 3, 2018

ankit-maverick commented Sep 6, 2018

ankit-maverick commented Sep 6, 2018 • edited Loading

carver commented Sep 6, 2018 • edited Loading

ankit-maverick commented Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voith commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

ankit-maverick commented Sep 15, 2018

ankit-maverick commented Sep 15, 2018

voith Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankit-maverick commented Sep 16, 2018

Choose a reason for hiding this comment

ankit-maverick Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

carver commented Sep 18, 2018

ankit-maverick commented Sep 19, 2018

ankit-maverick commented Sep 20, 2018

carver commented Sep 20, 2018

ccolorado commented Oct 15, 2018

carver commented Oct 15, 2018

ankit-maverick commented Aug 24, 2018 •

edited

Loading

ankit-maverick Aug 25, 2018 •

edited

Loading

ankit-maverick commented Aug 25, 2018 •

edited

Loading

pipermerriam commented Aug 27, 2018 •

edited by carver

Loading

ankit-maverick commented Sep 6, 2018 •

edited

Loading

carver commented Sep 6, 2018 •

edited

Loading

ankit-maverick commented Sep 9, 2018 •

edited

Loading

voith commented Sep 13, 2018 •

edited

Loading

voith Sep 15, 2018 •

edited

Loading

ankit-maverick Sep 18, 2018 •

edited

Loading