COO can downgrade his own offer before fulfilling it

[hashkitty]

Description

COO can make an attractive offer with high price for the kitty. When kitty owner approves token for Offers contract, COO can downgrade his offer and fulfill it on behalf of user.

Scenario

• COO creates new offer for the token
• Owner sees offer and starts acceptance process by submitting 'approve' transaction to Kitty Core contract
• As soon as owners transaction is mined, COO submits 3 transactions: cancelOffer, createOffer and fulfillOffer in sequence.
• If COO's transactions are mined before owner's fulfillOffer transaction, COO can get token at minimal price.

Impact

COO can trick user into selling valuable token at minimal price. Since COO can use another account in his control to make and cancel offer and also COO can be changed by CEO any time, user cannot detect or prevent this kind of fraud in advance. High impact, low likelihood.

Reproduction

    //create initial offer by COO account
   const tokenId = 1;
    let minimalValue = await offersContract.minimumTotalValue();
    await offersContract.createOffer(tokenId, { value: 1000 * minimalValue, from: coo });

    // wait for owner to approve token, cancel initial offer and create new one
    await offersContract.cancelOffer(tokenId, { from: coo });
    await offersContract.createOffer(tokenId, { value: minimalValue, from: coo });
        
    // accept offer for owner and make sure token transfered
    await offersContract.fulfillOffer(tokenId, 0, { from: coo });
    const newOwner = await nftTokens.ownerOf(tokenId);
    assert(newOwner === coo);

Fix

Either user should be able to set minimal acceptable price for the token before approving token for Offers contract or deny COO fulfilling offers on user behalf.

[pauliax]

This fix will not prevent COO from the possible manipulation. He could use another address to help him (create, cancel and re-create orders), so I think the ability to fulfil the order on user's behalf should be removed altogether.

[hashkitty]

pretty much duplicate of #22 but on closer look 22 is not about malicious COO. I'll live this open just in case, feel free to close if it does not add any value. I've update fix as @pauliax stated original proposed fix is wrong.

This issue is addressed by the _minOfferPrice parameter. Once CK addresses Issue #22 and has the user communicate _minOfferPrice on chain and holds COO to it, the _minOfferPrice parameter will sufficiently address your concern.

[hashkitty]

I agree that writing minOfferPrice to the chain before COO can fulfill offer will help. But I am not sure if it is most elegant solution, since it would require an extra interaction from owner side. As this is low likehood issue (even for rare high-valued tokens it is still unlikely that COO will commit fraud) I suggest that it should not degrade user experience in most cases.

I still want to suggest that contract is implemented in a way that owner has priority in fulfilling the offer. Description says "COO fulfills the offer as soon as our off-chain checks pass", not sure what that practically means but seems like COO is not intended to fulfill offer immediately after creation anyway. If contract has additional checks that do not allow COO to complete offer for smth like 300 blocks after creation, it could mitigate the risk pretty well imo.