Skip to content

Commit 2d232eb

Browse files
charles-coopertserg
charles-cooper
and
tserg
authored
feat[lang]: protect external calls with keyword (#2938)
this commit adds `extcall` and `staticcall` keywords to the vyper language. these are now a requirement for the user to add to distinguish internal calls from: 1) external calls which can have side effects (`extcall`), and 2) external calls which are guaranteed by the EVM to not have side effects (`staticcall`). `extcall` is used for `nonpayable` or `payable` functions (which emit the `CALL` opcode), while `staticcall` is used for `view` and `pure` functions (which emit the `STATICCALL` opcode). the motivation for this is laid out more in the linked GH issue, but it is primarily to make it easier to read, audit and analyze vyper contracts, since you can find the locations of external calls in source code using text-only techniques, and do not need to analyze (or have access to the results of an analysis) in order to find where external calls are. (note that this has become a larger concern with with the introduction of modules in vyper, since you can no longer distinguish between internal and external calls just by looking for the `self.` prefix). an analysis of some production contracts indicates that the frequency of external calls has somewhat high variability, but is in the range of one `extcall` (or `staticcall`) per 10-25 (logical) sloc, with `staticcalls` being about twice as common. therefore, based on the semantic vs write load of the keyword, the keyword should be somewhat easy to type, but it also needs to be long enough and unusual enough to stand out in a text editor. the differentiation between `extcall` and `staticcall` was added because, during testing of the feature, it was found that being able to additionally infer at the call site whether the external call can have side effects or not (without needing to reference the function definition) substantially enhanced readability. refactoring/misc updates: - update and clean up the grammar, especially the `variable_access` rule (cf. https://github.com/lark-parser/lark/blob/706190849ee/lark/grammars/python.lark#L192) - add a proper .parent property to VyperNodes - add tokenizer changes to make source locations are correct - ban standalone staticcalls - update tests -- in some cases, because standalone staticcalls are now banned, either an enclosing assignment was added or the mutability of the interface was changed. - rewrite some assert_compile_failed to pytest.raises() along the way - remove some dead functions cf. GH issue / VIP 2856 --------- Co-authored-by: tserg <[email protected]>
1 parent 327e95a commit 2d232eb

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

54 files changed

+946
-478
lines changed

‎examples/factory/Exchange.vy‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ def __init__(_token: IERC20, _factory: Factory):
2020
@external
2121
def initialize():
2222
# Anyone can safely call this function because of EXTCODEHASH
23-
self.factory.register()
23+
extcall self.factory.register()
2424

2525

2626
# NOTE: This contract restricts trading to only be done by the factory.
@@ -31,12 +31,12 @@ def initialize():
3131
@external
3232
def receive(_from: address, _amt: uint256):
3333
assert msg.sender == self.factory.address
34-
success: bool = self.token.transferFrom(_from, self, _amt)
34+
success: bool = extcall self.token.transferFrom(_from, self, _amt)
3535
assert success
3636

3737

3838
@external
3939
def transfer(_to: address, _amt: uint256):
4040
assert msg.sender == self.factory.address
41-
success: bool = self.token.transfer(_to, _amt)
41+
success: bool = extcall self.token.transfer(_to, _amt)
4242
assert success

‎examples/factory/Factory.vy‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,12 @@ def register():
3535
# NOTE: Should do checks that it hasn't already been set,
3636
# which has to be rectified with any upgrade strategy.
3737
exchange: Exchange = Exchange(msg.sender)
38-
self.exchanges[exchange.token()] = exchange
38+
self.exchanges[staticcall exchange.token()] = exchange
3939

4040

4141
@external
4242
def trade(_token1: IERC20, _token2: IERC20, _amt: uint256):
4343
# Perform a straight exchange of token1 to token 2 (1:1 price)
4444
# NOTE: Any practical implementation would need to solve the price oracle problem
45-
self.exchanges[_token1].receive(msg.sender, _amt)
46-
self.exchanges[_token2].transfer(msg.sender, _amt)
45+
extcall self.exchanges[_token1].receive(msg.sender, _amt)
46+
extcall self.exchanges[_token2].transfer(msg.sender, _amt)

‎examples/market_maker/on_chain_market_maker.vy‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ totalTokenQty: public(uint256)
88
# Constant set in `initiate` that's used to calculate
99
# the amount of ether/tokens that are exchanged
1010
invariant: public(uint256)
11-
token_address: IERC20
11+
token: IERC20
1212
owner: public(address)
1313

1414
# Sets the on chain market maker with its owner, initial token quantity,
@@ -17,8 +17,8 @@ owner: public(address)
1717
@payable
1818
def initiate(token_addr: address, token_quantity: uint256):
1919
assert self.invariant == 0
20-
self.token_address = IERC20(token_addr)
21-
self.token_address.transferFrom(msg.sender, self, token_quantity)
20+
self.token = IERC20(token_addr)
21+
extcall self.token.transferFrom(msg.sender, self, token_quantity)
2222
self.owner = msg.sender
2323
self.totalEthQty = msg.value
2424
self.totalTokenQty = token_quantity
@@ -33,14 +33,14 @@ def ethToTokens():
3333
eth_in_purchase: uint256 = msg.value - fee
3434
new_total_eth: uint256 = self.totalEthQty + eth_in_purchase
3535
new_total_tokens: uint256 = self.invariant // new_total_eth
36-
self.token_address.transfer(msg.sender, self.totalTokenQty - new_total_tokens)
36+
extcall self.token.transfer(msg.sender, self.totalTokenQty - new_total_tokens)
3737
self.totalEthQty = new_total_eth
3838
self.totalTokenQty = new_total_tokens
3939

4040
# Sells tokens to the contract in exchange for ether
4141
@external
4242
def tokensToEth(sell_quantity: uint256):
43-
self.token_address.transferFrom(msg.sender, self, sell_quantity)
43+
extcall self.token.transferFrom(msg.sender, self, sell_quantity)
4444
new_total_tokens: uint256 = self.totalTokenQty + sell_quantity
4545
new_total_eth: uint256 = self.invariant // new_total_tokens
4646
eth_to_send: uint256 = self.totalEthQty - new_total_eth
@@ -52,5 +52,5 @@ def tokensToEth(sell_quantity: uint256):
5252
@external
5353
def ownerWithdraw():
5454
assert self.owner == msg.sender
55-
self.token_address.transfer(self.owner, self.totalTokenQty)
55+
extcall self.token.transfer(self.owner, self.totalTokenQty)
5656
selfdestruct(self.owner)

‎examples/tokens/ERC4626.vy‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def transferFrom(sender: address, receiver: address, amount: uint256) -> bool:
7979
@view
8080
@external
8181
def totalAssets() -> uint256:
82-
return self.asset.balanceOf(self)
82+
return staticcall self.asset.balanceOf(self)
8383

8484

8585
@view
@@ -91,7 +91,7 @@ def _convertToAssets(shareAmount: uint256) -> uint256:
9191

9292
# NOTE: `shareAmount = 0` is extremely rare case, not optimizing for it
9393
# NOTE: `totalAssets = 0` is extremely rare case, not optimizing for it
94-
return shareAmount * self.asset.balanceOf(self) // totalSupply
94+
return shareAmount * staticcall self.asset.balanceOf(self) // totalSupply
9595

9696

9797
@view
@@ -104,7 +104,7 @@ def convertToAssets(shareAmount: uint256) -> uint256:
104104
@internal
105105
def _convertToShares(assetAmount: uint256) -> uint256:
106106
totalSupply: uint256 = self.totalSupply
107-
totalAssets: uint256 = self.asset.balanceOf(self)
107+
totalAssets: uint256 = staticcall self.asset.balanceOf(self)
108108
if totalAssets == 0 or totalSupply == 0:
109109
return assetAmount # 1:1 price
110110

@@ -133,7 +133,7 @@ def previewDeposit(assets: uint256) -> uint256:
133133
@external
134134
def deposit(assets: uint256, receiver: address=msg.sender) -> uint256:
135135
shares: uint256 = self._convertToShares(assets)
136-
self.asset.transferFrom(msg.sender, self, assets)
136+
extcall self.asset.transferFrom(msg.sender, self, assets)
137137

138138
self.totalSupply += shares
139139
self.balanceOf[receiver] += shares
@@ -153,7 +153,7 @@ def previewMint(shares: uint256) -> uint256:
153153
assets: uint256 = self._convertToAssets(shares)
154154

155155
# NOTE: Vyper does lazy eval on `and`, so this avoids SLOADs most of the time
156-
if assets == 0 and self.asset.balanceOf(self) == 0:
156+
if assets == 0 and staticcall self.asset.balanceOf(self) == 0:
157157
return shares # NOTE: Assume 1:1 price if nothing deposited yet
158158

159159
return assets
@@ -163,10 +163,10 @@ def previewMint(shares: uint256) -> uint256:
163163
def mint(shares: uint256, receiver: address=msg.sender) -> uint256:
164164
assets: uint256 = self._convertToAssets(shares)
165165

166-
if assets == 0 and self.asset.balanceOf(self) == 0:
166+
if assets == 0 and staticcall self.asset.balanceOf(self) == 0:
167167
assets = shares # NOTE: Assume 1:1 price if nothing deposited yet
168168

169-
self.asset.transferFrom(msg.sender, self, assets)
169+
extcall self.asset.transferFrom(msg.sender, self, assets)
170170

171171
self.totalSupply += shares
172172
self.balanceOf[receiver] += shares
@@ -206,7 +206,7 @@ def withdraw(assets: uint256, receiver: address=msg.sender, owner: address=msg.s
206206
self.totalSupply -= shares
207207
self.balanceOf[owner] -= shares
208208

209-
self.asset.transfer(receiver, assets)
209+
extcall self.asset.transfer(receiver, assets)
210210
log IERC4626.Withdraw(msg.sender, receiver, owner, assets, shares)
211211
return shares
212212

@@ -232,7 +232,7 @@ def redeem(shares: uint256, receiver: address=msg.sender, owner: address=msg.sen
232232
self.totalSupply -= shares
233233
self.balanceOf[owner] -= shares
234234

235-
self.asset.transfer(receiver, assets)
235+
extcall self.asset.transfer(receiver, assets)
236236
log IERC4626.Withdraw(msg.sender, receiver, owner, assets, shares)
237237
return assets
238238

@@ -241,4 +241,4 @@ def redeem(shares: uint256, receiver: address=msg.sender, owner: address=msg.sen
241241
def DEBUG_steal_tokens(amount: uint256):
242242
# NOTE: This is the primary method of mocking share price changes
243243
# do not put in production code!!!
244-
self.asset.transfer(msg.sender, amount)
244+
extcall self.asset.transfer(msg.sender, amount)

‎examples/tokens/ERC721.vy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ def safeTransferFrom(
248248
"""
249249
self._transferFrom(_from, _to, _tokenId, msg.sender)
250250
if _to.is_contract: # check if `_to` is a contract address
251-
returnValue: bytes4 = ERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data)
251+
returnValue: bytes4 = extcall ERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data)
252252
# Throws if transfer destination is a contract which does not implement 'onERC721Received'
253253
assert returnValue == method_id("onERC721Received(address,address,uint256,bytes)", output_type=bytes4)
254254

‎setup.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
"py-evm>=0.7.0a1,<0.8",
1919
"web3==6.0.0",
2020
"tox>=3.15,<4.0",
21-
"lark==1.1.2",
21+
"lark==1.1.9",
2222
"hypothesis[lark]>=5.37.1,<6.0",
2323
"eth-stdlib==0.2.6",
2424
],

‎tests/functional/builtins/codegen/test_abi_decode.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ def get_counter() -> Bytes[128]: nonpayable
244244
def foo(addr: address) -> (uint256, String[5]):
245245
a: uint256 = 0
246246
b: String[5] = ""
247-
a, b = _abi_decode(Foo(addr).get_counter(), (uint256, String[5]), unwrap_tuple=False)
247+
a, b = _abi_decode(extcall Foo(addr).get_counter(), (uint256, String[5]), unwrap_tuple=False)
248248
return a, b
249249
"""
250250

‎tests/functional/builtins/codegen/test_abi_encode.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ def get_counter() -> (uint256, String[6]): nonpayable
281281
282282
@external
283283
def foo(addr: address) -> Bytes[164]:
284-
return _abi_encode(Foo(addr).get_counter(), method_id=0xdeadbeef)
284+
return _abi_encode(extcall Foo(addr).get_counter(), method_id=0xdeadbeef)
285285
"""
286286

287287
c2 = get_contract(contract_2)

‎tests/functional/builtins/codegen/test_addmod.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@ def test_uint256_addmod_ext_call(
1919
w3, side_effects_contract, assert_side_effects_invoked, get_contract
2020
):
2121
code = """
22-
@external
23-
def foo(f: Foo) -> uint256:
24-
return uint256_addmod(32, 2, f.foo(32))
25-
2622
interface Foo:
2723
def foo(x: uint256) -> uint256: payable
24+
25+
@external
26+
def foo(f: Foo) -> uint256:
27+
return uint256_addmod(32, 2, extcall f.foo(32))
2828
"""
2929

3030
c1 = side_effects_contract("uint256")

‎tests/functional/builtins/codegen/test_as_wei_value.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,12 @@ def foo(a: {data_type}) -> uint256:
9797

9898
def test_ext_call(w3, side_effects_contract, assert_side_effects_invoked, get_contract):
9999
code = """
100-
@external
101-
def foo(a: Foo) -> uint256:
102-
return as_wei_value(a.foo(7), "ether")
103-
104100
interface Foo:
105101
def foo(x: uint8) -> uint8: nonpayable
102+
103+
@external
104+
def foo(a: Foo) -> uint256:
105+
return as_wei_value(extcall a.foo(7), "ether")
106106
"""
107107

108108
c1 = side_effects_contract("uint8")

0 commit comments

Comments
 (0)