SettleFundsExpired is broken, therefore closing markets is broken

[Henry-E]

Unfortunately it looks like @binyebarwe added the requirement for both the account owner and penalty account to sign the settle funds expired function after @skrrb originally created the function without these signers.

https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/src/accounts_ix/settle_funds_expired.rs#L8-L13

The whole point of expired functions is to allow the close market admin to clean up a market and get it ready to close. First using the prune orders function, then settling those accounts so the market vaults are empty, and finally closing the market.

e.g. here no signature from the owner of the open orders account is needed to prune their orders because the market has expired. https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/src/accounts_ix/prune_orders.rs#L6-L12

Essentially this means the whole close market admin functionality is broken. Since it's not possible to close the market and retrieve sol rent back unless all the users have settled their own accounts.

Perhaps I'm missing something and it is still possible to settle accounts and close the market?

Edit: In the commit where the payer was added I can see the logic is an improvement, since the original function was assuming the close_market_admin was also the account owner, which is not a good assumption to make. But yeh, get rid of the signing requirement please.

[skrrb]

urgh, it's not broken but super-confusing...

Note that in settle_funds_expired ix, owner account is not used at all. Well, it's used here https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/src/accounts_ix/settle_funds_expired.rs#L58

but given that the owner is only employed during settle_funds account validations, e.g. https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/src/accounts_ix/settle_funds.rs#L16

but nowhere within the instruction, actually any account could be passed. Indeed, there's one working test that signs with a different key than the open orders' owner:

https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/tests/cases/test_permissioned.rs#L369

so i'm going to close this issue, but please feel free to reopen it if you think the above doesn't address the problem or you have any further questions

[Henry-E]

Goddamn you're right.

It's crazy right, that there are two signers there which are never checked or used for anything? Why even have them there. At best these should just be deleted in the next breaking version change.

[skrrb]

only the owner signer is unnecessary, penalty_payer is required for this transfer

https://github.com/openbook-dex/openbook-v2/blob/c8895703f4ac9d11361b6a56b68491879a622a78/programs/openbook-v2/src/instructions/settle_funds.rs#L41

but yeah, i agree that it should be cleaned up (with a lot of caution to not break potential existing integrations)

[Henry-E]

Also worth double checking that in permissioned markets the penalty payer doesn't become a blocker to closing. Essentially the whole point of pruning, settling and closing expired markets is for the market runner to be able to safely shutdown markets and get down rent without giving griefers the option to leave unsettled accounts that prevent the markets from closing.

I'm not familiar with the penalty payer or how it works, so sorry if my comment doesn't make sense.