safe guards

[raedah]

Related to the recent security issues, Id like to have more safe guards in place. For starters, Id like a feature that operators of yield generators can enable to create only positive yield transactions. The check would have to be done at the very last step, when validating the transactions just before signing it. If the transaction does not create a positive yield, then the yield generator would immediately error and quit.

Another possible safe guard would be to require the total balance to stay above the same amount that existed when the yield generator launched. If the total amount was to fall at all, then the yield generator would error and quit. This feature could be opt in enablable initially.

[adlai]

I'm morally opposed to forcing yield generators to actually generate yield. If people are stupid or generous enough to configure negative fees, they deserve them.

Soft-NACK, because there are other dictators on this ship.

[chris-belcher]

What's the recent security issue ?

@adlai there may also be bugs in JoinMarket that causes people to lose money through no fault of their own.

[adlai]

@chris-belcher I think @raedah was talking about the issue which didn't affect yield generators.

[raedah]

Right, it didnt effect yield generators. Still, it just an eye opener to potential vulnerabilities, and we should have more fail safes in place.

I'm morally opposed to forcing yield generators to actually generate yield.

I agree with you. I believe there might be actual use cases where people would want a negative yield. What I am proposing here is a user configurable limit, default net 0 or max negative net 2%, that screens all transactions before signed.

The reason we need this is because currently we have a check_high_fee function in the taker code and what I learned in IRC was that "the bug bypasses that check." So lets put the check in a place that would actually protect funds, and while we are at it, lets extend it to protect makers as well as takers.

[AdamISZ]

As I recall there are sanity checks on both sides that the transaction to be signed satisfies what the person requested. The "bug bypassed the check" basically means: the fee to be checked was being read from the current fields of the order in the database, instead of the value of the coinjoin fee at the time the transaction was accepted (see https://github.com/JoinMarket-Org/joinmarket/commit/508d65f9a9c3f7ad7c542e11d19148c91b89e943#diff-e5ebbf3237752f0c78e5834760582020L112)

The fix locks the content of the order at the time of agreement, instead of re-reading it from db.

The check_high_fee is doing something else: checking that the total fee requested, at the time of order acceptance, is within our chosen sanity bounds. Your suggestion of a user-configurable yield generator minimum expected yield is at the same level as that, but it seems to me we already have such things (at least in the simple versions of the yield gen?).

On the general topic, it's certainly worthwhile to look for ways in which we could make more robust checks that nothing is wrong with the transaction. I remember saying very early on "wouldn't it be good to isolate the transaction signing in a very explicit way", kind of in the same spirit: to lock down what spends are signed. But just as a vague concept it isnt that useful, except perhaps architecturally. Just isolating the code wouldn't have picked up that bug, what's needed is to actually make sure one understands what every step of the code is doing (I myself just glossed over that db read and never took any notice of it.. :( )

[raedah]

"what's needed is to actually make sure one understands what every step of the code is doing" I disagree that we should rely on the code working as expected. The bug in case is a perfect example of why. The point of a safe guard is to have additional checks when all else fails.

If taker.py CoinJoinTX() had a def sanity_check() which was called perhaps before add_signature(), and maker.py CoinJoinOrder), had a def sanity_check() which was called before perhaps verify_unsigned_tx()

The sanity check would fail if conditions are not meant, such as, we are quite sure that if the yield of a transaction is greater then negative 2%, there is likely something wrong.

Something like this would have offered protection from the recent issue which put all of a users funds at risk. It would also alert us to there being a problem when the error occurs.

[AdamISZ]

Yes, but that's precisely my point: I agree that extra sanity checks are a good thing, but I'm pointing out it wouldn't have helped here, because the db re-read overwrote what the sanity check would check for! :)

[raedah]

You are talking about checking the value before it goes into the db. Yes it makes sense to check data when it is received, before we put it into the database. I am talking about a different safe guard, that checks the data at the end. It checks it before it is sent. If I can make the time to do this with code, I will show you.

[raedah]

There is some confusion on this bug, and it seems there is not a single piece of written data about what this exploit was or what the resolution was. I know this is open source as is software and we are all volunteers. Just pointing it out as a team that this could be valuable for us.

[chris-belcher]

Here is a quick summary by me.

The IRC thread receives orders from IRC and update the orderbook database, sendpayment and tumbler create another thread that invokes the taker routines. The exploit was that the maker could force that thread to wait by delaying the response of !sig and meanwhile sending out new offers with a higher price. The IRC thread would update the database and when the maker finally sent !sig, the sendpayment thread would read the new offer from the database and use it to construct the transaction. This exploit was found by someone who idles on #joinmarket IRC who messaged me in private to tell me about it

[AdamISZ]

You are talking about checking the value before it goes into the db

No, that's not quite it.

@raedah the reason that the commit that fixed it is a bit obscure looking is, it required rewriting what data is passed into the CoinjoinTx constructor; the "order" previously was effectively just a pointer to a certain maker's order entry in the database, and then the database was re-checked to retrieve the coinjoin fee. The problem with that is that the database of orders is maintained separately in the irc thread, and it could be updated between the time the taker first agreed to it, and the time that the utxos from the maker were delivered. (the maker could reannounce the order with a different fee)

In order to fix this it was necessary to rewrite the coinjointx constructor so that the 'orders' variable included the full details of the order, rather than rereading them. Does that help?

The commit is here: https://github.com/JoinMarket-Org/joinmarket/commit/508d65f9a9c3f7ad7c542e11d19148c91b89e943

see in particular this line

The large amount of changes is due to that structural change propagating.