bugs in binary-oracle-pair

[aeyakovenko]

Is this the same thing? https://github.com/solana-labs/solana-program-library/tree/master/binary-oracle-pair

No, as far as I can tell, these programs are sufficiently different. I'll try to respond as objectively as I can by reading through that source code.

At first glance, the structures are similar, but I'm not getting any notions of the actual "trading" of positions from the binary-oracle-pair module. I wrote a really detailed README describing the mechanics of how this betting pool protocol manages transactions between 2 parties based on their different starting positions (proper management of position transfer is important because in real-life betting markets, decisions are binding and you can only exit when another person as agreed to take on your risk):

https://github.com/jarry-xiao/solana-program-library/blob/betting-pool-primitive/betting-pool/

Here is my understanding of how the binary-oracle-pair works:

InitPool Creates a pool object with metadata, initializes mints for a PASS and FAIL token. seems like you can only mint new tokens before the mint_end_slot, you can only decide between the mint_end_slot and the decide_end_slot

Deposit If you are in a slot < mint_end_slot you can transfer some of your tokens into a deposit_account and the protocol mints an equal number of PASS and FAIL tokens.

Withdraw If the pool is decided there are 3 cases.

1) PASS: You burn tokens from some account (assumed to be the pass_token_account, but there is NO MINT CHECK) and then receive an equivalent amount of collateral back.

2 FAIL: You burn tokens from some account (assumed to be the pass_token_account, but there is NO MINT CHECK) and then receive an equivalent amount of collateral back.

3) UNDECIDED: If a decision hasn't been reached yet, you can burn all of your "fail" and "pass" tokens (again no mint check) and pull back an equivalent amount of collateral.

Decide will decide the winner if there is one (assuming mint_end < slot_time < deposit_end)

I can sort of see how you can turn this in to a betting pool-esque protocol, but IMO it's convoluted. A deposits x, and B deposits y to an escrow account, escrow invokes Deposit with amount x + y into the pool, A receives x+y PASS tokens and B receives x+y FAIL tokens. This higher level logic still needs to be handled somewhere.

In isolation (ignoring rug possibility), there is no risk in calling Deposit, because you can always pull out the same amount you deposited assuming that you don't burn or transfer your FAIL or PASS tokens. There's only risk if they are distributed (like in the above escrowed bet example between A and B, A risks x to win x+y and B risks y to win x+y)

Question:

Isn't the Withdraw function essentially just a rug (pass in a burner tokens, collect honeypot). I think it allows you to burn any token type (pass/fail never checked AFAICT)? The burn and transfer functions don't actually use the PDA to sign, so my attack would be to mint 2 random tokens in account I control, pass them into Withdraw, and collect the very real collateral stored in the pool deposit account. Only safety is whether you have the proper authority_id but I think you can derive that with a client library as you know the base seeds used for that PDA. Happy to be wrong here, maybe there's something in the source that prevents this that I don't know about.

TL;DR: The betting pool protocol deals with users transacting with different bets and has complex logic wrapping this. binary-oracle-pair can be used as the core "pool" primitive, but has 0 position management logic implemented.

The betting-pool also opens up betting indefinitely and leaves settlement to an higher level protocol. binary-oracle-pair has slot time constraints.

Originally posted by @jarry-xiao in https://github.com/solana-labs/solana-program-library/pull/2056#issuecomment-877658305

[aeyakovenko]

Tag @jstarry @joncinque wdyt?

[joncinque]

Oops, yeah that's definitely a bug to fix. Nice spot @jarry-xiao !

[jordaaash]

@jarry-xiao can this be closed?

[jordaaash]

Closing as stale, I believe this was resolved in https://github.com/solana-labs/solana-program-library/pull/2056

[joncinque]

I don't think this was resolved unfortunately, there's still the bug in the binary-oracle-pair program where the mints aren't checked:

Isn't the Withdraw function essentially just a rug (pass in a burner tokens, collect honeypot). I think it allows you to burn any token type (pass/fail never checked AFAICT)? The burn and transfer functions don't actually use the PDA to sign, so my attack would be to mint 2 random tokens in account I control, pass them into Withdraw, and collect the very real collateral stored in the pool deposit account. Only safety is whether you have the proper authority_id but I think you can derive that with a client library as you know the base seeds used for that PDA. Happy to be wrong here, maybe there's something in the source that prevents this that I don't know about.

Essentially, very few of the inputs around https://github.com/solana-labs/solana-program-library/blob/fd662e5f878d58ad06d3cb6365171ebaec41b39d/binary-oracle-pair/program/src/processor.rs#L403 are checked :sweat_smile:

[jordaaash]

Ah, sorry about that! I misunderstood

[jarry-xiao]

Added a fix

[joncinque]

@jarry-xiao This should be ready to close, correct?

[joncinque]

Closing, I think these bugs were fixed up