Putty contest - sseefried's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L228 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L240 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L436

Vulnerability details

Impact

When users call either fillOrder or exercise it is possible to send ETH to the contract accidentally. This happens when weth != order.baseAsset but msg.value > 0.

Also, the functions setFee and setBaseURI are marked as payable which further allows ETH to be sent to the contract.

Once this excess ETH is in the contract there is no way to get it back out since there are no functions to recover it.

Proof of Concept

• Let weth != order.baseAsset but have msg.value > 0

• When fillOrder is called line 338 or line 360 can be executed.

• When exerciseis called line 436 can be executed.

In all cases it is possible for msg.value > 0 to be true when these lines are executed. The order.baseAsset tokens are transferred but excess ETH is also locked in the contract.

Also setBaseURI and setFee can lock ETH since they are marked payable.

Recommended Mitigation Steps

Change the logic in the relevant places in fillOrder and exercise to something like the following:

if (weth == order.baseAsset && msg.value > 0) {
  ... deposit weth and transfer
} else {
  require(msg.value == 0, "ETH shouldn't be sent");
  ... transfer base asset ...
}

Fortunately, the expected behaviour of the Putty contract works in our favour for getting the ETH back out. Whenever ETH is sent as a substitute for order.baseAsset == weth the expected behaviour is that the ETH will be deposited into the weth contract and transferred to the contract. Thus, there should never be excess ETH in the Putty contract. Thus, any left-over ETH must have been put there by accident.

Simply add a function to withdraw it:

function withdrawExcessETH(address to) public onlyOwner {
  (bool success,) = to.call{value: address(this).balance}("");
  require(success, "ETH not sent");
}

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L454 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L601 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/lib/solmate/src/utils/SafeTransferLib.sol#L51 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L454 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L638

Vulnerability details

This submission contains a link to a private fork of the contest repo. Please contact warden @sseefried for GitHub collaborator access.

Impact

A user can put in an order for a Long Put Option but -- either accidentally or not -- put an ERC721 asset in the erc20Assets field of an order. Because ERC20 (via SafeTransferLib) and ERC721 both implement the safeTransferFrom function (and with the same signature), the ERC721 asset is successfully transferred to the contract when exercise is called but fails to transfer out of the contract when withdraw is called.

The impact is that the ERC721 asset becomes locked in the Putty contract.

Proof of Concept

• Alice puts in an order for a Long Put but smuggles in an ERC721 asset with tokenId = 42 into the erc20Assets field i.e. Declare ERC20Asset erc20Asset and set erc20Asset.token = <ERC721 token address> and erc20Asset.tokenAmount = 42 (42 is also a valid amount).
• Bob fills the order
  • the premium is transferred from Alice to Bob
  • the strike price is transferred from Bob to the Putty contract
• Alice buys the asset -- presumably at a lower price than the strike but perhaps not if her sole intention is to grief -- and calls exercise
  • The strike price is transferred from Putty to Alice on line 451.
  • The ERC721 asset is successfully transferred from Alice to the Putty Contract. This is because:
    • line 454 is executed which further calls line 601 in _transferERC20In.
    • This calls the safeTransferFrom function from SafeTransferLib. This will perform a call to function ERC721.safeTransferFrom (which just so happens to have the same signature as ERC20.safeTransferFrom).
    • This call to ERC721.safeTransferFrom succeeds since Putty implements the onERC721Received function. Notice that there is no return value from ERC721.safeTransferFrom.
    • Line SafeTransferLib.sol:51 succeeds because iszero(returndatasize()) is true!
• Bob attempts to call withdraw.
  • line 510 is called which further calls line 638 in _transferERC20sOut.
  • This time SafeTransferLib.safeTransfer is called (not SafeTransferLib.safeTransferFrom). This attempts to call ERC721.safeTransfer which fails since this function doesn't exist. The result is that ERC721.safeTransfer reverts with message TRANSFER_FAILED and the ERC721 is locked in the Putty contract.

An executable PoC can be found here.

Recommended Mitigation Steps

Consider using OpenZeppelin's IERC165 contract to see if tokens passed in implement the ERC20 interface. Also see EIP165.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L268 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L240

Vulnerability details

Impact

Function setFee can be called at any time, including after fillOrder has been called. This could be deemed unfair to the user in the Short Position, since they created the order at a particular point in time.

The impact is that they may end up paying more fees than they expected when they call withdraw.

Proof of Concept

• User creates order and fee is currently 5 (i.e. 0.5%).
• Function fillOrder is called on the order.
• Owner calls setFee increasing it to 30 (i.e. 3%)
• User in short position calls withdraw (either when Call and Exercised, or when Put and not Exercised). They end up paying 3% in fees instead of 0.5%.

Recommended Mitigation Steps

Add a new mapping variable that records the fee at the time fillOrder is called.

mapping (uint256 => uint256) public positionFees;

Then make the following edits to fillOrder and withdraw.

Edit for function fillOrder:

        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
...
positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;
// Saves fee at at time fillOrder called
positionFees[order.isLong ? positionId : uint256(orderHash)] = fee;
...

Edit for function withdraw:

function withdraw(Order memory order) public {
...
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    uint256 positionFee = positionFees[uint256(orderHash)];
    if (positionFee > 0) {
        feeAmount = (order.strike * positionFee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);
    return;
}
...

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");