QA Report

[code423n4]

QA Report

Remarks/Recommendations

• Consider adding blacklisting functionality for rebase and fee-on-transfer tokens. It will really help prevent user error.

• Consider using a constant instead of 1000 as a magic number on line 499

• Consider using a constant instead of 30 as a magic number on line 241

• Functions setBaseURI andsetFee are both payable when they don't appear to need to be. See my Medium Risk submission for how this can lead to ETH being locked in the contract.

• Consider adding a blacklist to order for greater ease of permissioning. If there is even a single whitelist entry then only those people are permissioned. Perhaps you just want to block just one address?

• Rename setFee to setFeeRate. It's worth keeping those concepts distinct. Change NewFee to NewFeeRate. I think of "fee" as the amount and "fee rate" as the rate the fee is calculated with.

• I must commend you on adding line 719 to hashOrder. This ensures that address(this) is part of the input to the final hash. Without this, Replay Attacks would have been possible on a copy of the Putty contract.

Typos

• PuttyV2.sol:679 "it's" -> "its"

Documentation errors

• line 135 "Fee rate that is applied on exercise" -> "Fee rate that is applied on withdraw"

• On lines 562-563 we have:

  /**
  @notice Accepts a counter offer for an order. It fills the counter offer, and then
        cancels the original order that the counter offer was made for.
  ..
  */

  But the cancel comes first, and then the order is filled.

Low Risk: cancel should fail if fillOrder has been called

Impact

A user may falsely believe that they have cancelled an order even though it has already been filled with fillOrder, since cancel does not revert in this case.

Also a false CancelledOrder event is emitted in this case.

Proof of Concept

• User makes order.
• Function fillOrder is called on this order
• User now calls cancel and it succeeds, also emitting a CancelledOrder event.

Recommended Mitigation Steps

Make the following changes to function cancel:

function cancel(Order memory order) public {
    require(msg.sender == order.maker, "Not your order");
    bytes32 orderHash = hashOrder(order);
    uint256 filledExpiry = positionExpirations[order.isLong ? uint256(orderHash) : uint256(hashOppositeOrder(order))];
    require(filledExpiry == 0, "Order has already been filled");

    // mark the order as cancelled
    cancelledOrders[orderHash] = true;

    emit CancelledOrder(orderHash, order);
}

Low Risk: acceptCounterOffer should check that order is opposite of originalOrder and that order.maker is not the original order maker

Impact

Function acceptCounterOffer will cancel an original order and then call fillOrder on the counter-offer order. However, for this to work properly the counter-offer order must be the opposite kind of order. e.g. a Long if the original was a Short or vice versa. Additionally the maker of the counter-offer should not be the same as the maker of the original order.

If the order is the same kind as originalOrder then caller of acceptCounterOffer is putting themselves in the opposite position of where they wanted to be.

The impact is a potential for user error.

Proof of Concept

Let

• the original order be a Long Put.
• Alice be the original order maker
• Bob be the user that gives a counter-offer instead of calling fillOrder

If Bob had called fillOrder then Alice would become the recipient of a Long Put Option.

Case 1: Order kind is still Long Put

Instead Bob provides a counter-offer but also makes it a Long Put. He changes the order.maker to himself. Alice now calls acceptCounterOffer.

Line 583 will call fillOrder but it is being called by the original order maker. This is true because otherwise line 579 would revert.

This would be a very easy mistake for Alice to make if they weren't thinking carefully enough.

Unfortunately, she has now put herself in the Short Put position which is not what she desired and could be disadvantageous.

Case 2: originalOrder.maker == order.maker

If the original order maker is equal to the (counter-offer) order maker then when Alice calls fillOrder she now owns both position NFTs which is not desired behaviour.

It does not disadvantage her to any great extent as she can simple exercise and withdraw and will only lose some fees in the process as she will receive both the strike price and assets back.

Recommended Mitigation Steps

Add the following two lines to acceptCounterOffer to prevent these kinds of user error.

require(originalOrder.isCall == order.isCall &&
        originalOrder.isLong != order.isLong, "Counter-offer should be opposite");
require(originalOrder.maker != order.maker, "Counter-offer maker should be different");

[outdoteth]

high quality report