metaplex-foundation / metaplex-program-library

Smart contracts maintained by the Metaplex team
Other
588 stars 513 forks source link

Sell instruction implementation does not assert that there is exactly 1 signer (wallet or authority) #911

Closed serejke closed 1 year ago

serejke commented 1 year ago

https://github.com/metaplex-foundation/metaplex-program-library/blob/e057c72645b32a5b3776a6a93e33440e719a0ddd/auction-house/program/src/sell/mod.rs#L364

There must be one and only one signer; there can never be zero signers or two signers.

Sell instruction's doc states that either wallet (seller) or authority must be the signers. But the smart contract's code allows to have both of them signers.

Because the only if condition:

if !wallet.to_account_info().is_signer
        && (buyer_price == 0
            || free_seller_trade_state.data_is_empty()
            || !authority.to_account_info().is_signer
            || !auction_house.can_change_sale_price)
    {
        return Err(AuctionHouseError::SaleRequiresSigner.into());
    }

passes when both wallet.to_account_info().is_signer and authority.to_account_info().is_signer are true.

Metaplex-JS library handles this by specifying only one of them as the signer.

// Signers.
  const signer = isSigner(seller) ? seller : (authority as Signer);

but I think both Metaplex-JS and smart contract should throw a corresponding exception clearly indicating the misuse.

My use case: I want the seller to create a listing (sell) with price > 0, but the seller might not have native SOLs to pay for the seller trade state account creation.

lorisleiva commented 1 year ago

Hi there 👋

Thanks for raising this.

I did a bit of digging and when passing both the seller and the authority as signers:

I might be missing something but, since approving a delegate authority on the SPL Token program does not create a new account, the seller shouldn't need to have any native SOLs to pay for any storage since the authority is the fee payer. Thus, adding an extra check enforcing comment number 3 probably wouldn't fix your use case.

That being said, I agree that we should check for the invariants we expect instead of being okay with the fallback behaviour to avoid bad surprises in the future.