Vyper: Is Safe Remote Purchase vulnerable to a recursive call attack (answer: no, but let's keep things consistent) ?

[sgryphon]

Version Information

• vyper Version: latest (current docs)
• OS: n/a
• Python Version (python --version): n/a
• Environment (output of pip freeze): n/a

What's your issue about?

In Vyper Safe Remote Purchase example, the buyer has put up an extra deposit, so it is in their interest to acknowledge the item was received.

@public
def received():
    assert not self.unlocked #Is the item already purchased and pending confirmation
        # from the buyer?
    assert msg.sender == self.buyer
    send(self.buyer, self.value) #Return the buyer's deposit (=value) to the buyer.
    selfdestruct(self.seller) #Return the seller's deposit (=2*value)
        # and the purchase price (=value) to the seller.

Earlier, in the auction example, it went to great lengths to describe the correct sequence of events:

    # 1. Conditions
    # 2. Effects
    # 3. Interaction        

But then in the Safe Remote Purchase example, does it not break those rules -- could the buyer not recursively call received(), inside a contract address that is receiving the send of the value?

Then, because selfdestruct has not yet run, they can call received 4 times, leaving nothing for the seller?

How can it be fixed?

Follow the 1/2/3 rules.

Cute Animal Picture

put a cute animal picture here.

Um.

[jacqueswww]

@sgryphon @fubuloubu this example makes we wonder if there is room to build a @run_once decorator. Effectively under the hood to have a toggled state variable with an assert. Maybe even @run_only_once.

[jacqueswww]

@sgryphon in this example would the contract not be destroyed one level deep?

received() -> send() -> received() -> selfdestruct

[fubuloubu]

I think what he means is you could write a buyer contract that does this:

@payable
def __default__():
    self.srp.received()

[jacqueswww]

@fubuloubu yes I understand that but self.srp.received() would then call selfdestruct which would destroy the contract ?

[fubuloubu]

@jacqueswww I'd rather have it be a @multiple_calls_allowed acknowledging that you have done the analysis and are okay with multiple calls to this method.

There could be some internal integer that tracks whether a method ID has been accessed by seeing if bit n is set (where n is the first byte of the method id). You would reset that bit at the end of every call, assuring that that function was not accessed again while it was running. This acts as sort of a bloom filter. The decorator would avoid this extra check.

At the end of all contract execution, the integer would be 0.

[jacqueswww]

@fubuloubu I am afraid that would be way to expensive gas wise, as one would need to use storage to persist between calls, or use an inconsistent ABI call spec that contains this using call data.

[jacqueswww]

The idea of @run_once or @run_once(state_variable: bool) would be to have a cleaner and more concise way of implementing this boilerplate:

ended: bool

@public
def once():
      assert self.ended == False
      self.ended = True 

I really like the idea of not having to even have to expose it to the programmer, can still be read via ABI using some convention like function_name_ran_once.

@run_only_once
@public
def once():
     send() 
     selfdestruct()

[jacqueswww]

@fubuloubu @sgryphon I just did a test myself, to make sure I understand the mechanics. However assert msg.sender == self.buyer stops the re-entry beautifully because the call is from the purchase contract at that point.

[fubuloubu]

I would say the boilerplate doesn't actually solve anything, just makes it easier to solve if you are aware of it.

Awareness is key!