Reentrancy Bug in Market Maker Example

[cbraem]

What's your issue about?

The example examples/market_maker/on_chain_market_maker.vy seems to have a reentrancy bug. ethToTokens and tokensToEth first make an external function call before they update local state variables.

The effect of this reentrancy bug is demonstrated by the following example:

• initiate gets called with a token_quantity of 20 and msg.value of 100.

• Contract a calls tokensToEth with a sell_quantity of 40.

  • This contracts will send (20 * 100) / 40 = 50 back to a.
    • The default function of a calls now again tokensToEth with a sell_quantity of 40.
      • This contracts will send again 50 back to a.
        • a accepts now the 50 without further reentrant calls.
        • The second send to a returns.
      • This contract updates now totalEthQty to 50 and totalTokenQty to 60.
      • The reentrant call returns.
    • The first send to a returns.
  • This contract updates now totalEthQty to 50 and totalTokenQty to 40.

The contract has now 60 Tokens and 0 ether. Therefore, the invariant does not hold anymore and totalEthQty as well as totalTokenQty do no longer model the current state.

I assume that these examples just demonstrate what Vyper contracts can do, but I would rather suggest to fix such a reentrancy bug not to show case anti-patterns.

How can it be fixed?

Move the call to transfer at the end of ethToTokens and also move send at the end of tokensToEth.

[fubuloubu]

Yep, this appears to be true.

You suggested solution would definitely work to mitigate this, or it might also be a good spot to show off our @nonreentrant('name') decorator (our implementation of OpenZeppelin/ReentrancyGuard.sol), using the same name on both ethToTokens and tokensToEth to ensure that a reentrancy guard is created for both calls when either call is made.

If you want to fix it, let me know, otherwise I'll look into making this change when I get a chance.

[cbraem]

You're right, the nonreentrant decorator is even a better solution.

Since I only have an already built version of vyper and don't know everything that I should consider before making a commit / pull request, I would gratefully accept your offer to make this change.

[fubuloubu]

On further inspection, this is not true, due to the 2300 gas limit on send, checkout:

vyper/tests/parser/functions/test_send.py

Lines 31 to 72 in 1ebce95

	def test_default_gas(get_contract, w3):
	"""
	Tests to verify that send to default function will send limited gas (2300),
	but raw_call can send more.
	"""
	sender_code = """
	@public
	def test_send(receiver: address):
	send(receiver, 1)
	@public
	def test_call(receiver: address):
	raw_call(receiver, b"", gas=50000, max_outsize=0, value=1)
	"""
	# default function writes variable, this requires more gas than send can pass
	receiver_code = """
	last_sender: public(address)
	@public
	@payable
	def __default__():
	self.last_sender = msg.sender
	"""
	sender = get_contract(sender_code, value=1)
	receiver = get_contract(receiver_code)
	sender.test_send(receiver.address, transact={'gas': 100000})
	# no value transfer hapenned, variable was not changed
	assert receiver.last_sender() is None
	assert w3.eth.getBalance(sender.address) == 1
	assert w3.eth.getBalance(receiver.address) == 0
	sender.test_call(receiver.address, transact={'gas': 100000})
	# value transfer hapenned, variable was changed
	assert receiver.last_sender() == sender.address
	assert w3.eth.getBalance(sender.address) == 0
	assert w3.eth.getBalance(receiver.address) == 1

---

It may be time to re-evaluate this mechanic, and explore using a "default reentrancy guard" on all outgoing sends instead (protecting against reentrancy without using a broken gas mechanic that may have a silent failure)

cc @iamdefinitelyahuman thoughts?

[iamdefinitelyahuman]

Relying on the 2300 gas sent seems bad, yeah. And limiting the gas to 2300 is also a bit outdated and we should rethink that IMO: https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/

I'm not sure how I feel about a default re-entrancy guard either but definitely into having a conversation about it.

[fubuloubu]

Lol, already had this discussion: #1756