Putty contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

Fees are expected to be paid whenever an option is exercised (as per the function comment on L235).

However, the current protocol implementation also charges fees for expired put options. The owner of a short put option is subject to paying fees whenever the strike is returned for expired put options.

Proof of Concept

PuttyV2.withdraw

// transfer strike to owner if put is expired or call is exercised
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    if (fee > 0) {
        feeAmount = (order.strike * fee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); // @audit-info fee should not be paid if strike is simply returned to short owner for expired put options

    return;
}

Tools Used

Manual review

Recommended mitigation steps

Do not charge fees for expired put options by adapting the withdraw (L494-L506) implementation as following:

// transfer strike to owner if put is expired or call is exercised
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;

    // @audit-info only charge fee if call is exercised
    if ((order.isCall && isExercised) && fee > 0) {
        feeAmount = (order.strike * fee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

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

    return;
}

Lines of code

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

Vulnerability details

Impact

Fees are expected to be paid whenever an option is exercised (as per the function comment on L235).

Put options

If a put option is exercised, the exerciser receives the strike price (initially deposited by the short position holder) denominated in order.baseAsset.

Call options

If a call option is exercised, the exerciser sends the strike price to Putty and the short position holder is able to withdraw the strike amount.

However, the current protocol implementation is missing to deduct fees for exercised put options. Put options are free of any fees.

Proof of Concept

The protocol fee is correctly charged for exercised calls:

PuttyV2.withdraw

// transfer strike to owner if put is expired or call is exercised
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    if (fee > 0) {
        feeAmount = (order.strike * fee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); // @audit DoS due to reverting erc20 token transfer (weird erc20 tokens, blacklisted or paused owner; erc777 hook on owner receiver side can prevent transfer hence reverting and preventing withdrawal) - use pull pattern @high  // @audit zero value token transfers can revert. Small strike prices and low fee can lead to rounding down to 0 - check feeAmount > 0 @high  // @audit should not take fees if renounced owner (zero address) as fees can not be withdrawn @medium
    }

    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); // @audit fee should not be paid if strike is simply returned to short owner for expired put @high

    return;
}

Contrary, put options are free of any fees:

PuttyV2.sol#L450-L451

// transfer strike from putty to exerciser
ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

Tools Used

Manual review

Recommended mitigation steps

Charge fees also for exercised put options.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L322-L340 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L350-L361 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L422-L443 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L443-L457

Vulnerability details

Impact

A user can choose to pay the option premium and strike with native ETH. In this case, the received ETH is converted to WETH.

However, if a user accidentally sends ETH to the fillOrder or exercise function without having WETH as the order.baseAsset, the sent ETH funds are lost and can not be recovered.

Proof of Concept

fillOrder

There are 2 ways to accidentally lose ETH as a taker:

1. Filling a short position with order.baseAsset != weth, taker accidentally sends ETH to fillOrder

   PuttyV2#L322-L340

   // transfer premium to whoever is short from whomever is long
   if (order.isLong) {
       ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
   } else {
       // handle the case where the user uses native ETH instead of WETH to pay the premium
       if (weth == order.baseAsset && msg.value > 0) {
           // check enough ETH was sent to cover the premium
           require(msg.value == order.premium, "Incorrect ETH amount sent");
   
           // convert ETH to WETH and send premium to maker
           // converting to WETH instead of forwarding native ETH to the maker has two benefits;
           // 1) active market makers will mostly be using WETH not native ETH
           // 2) attack surface for re-entrancy is reduced
           IWETH(weth).deposit{value: msg.value}();
           IWETH(weth).transfer(order.maker, msg.value);
       } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
           ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
       }
   }

2. Filling a long put position with order.baseAsset != weth, taker accidentally sends ETH to fillOrder

   PuttyV2#L350-L361

   // handle the case where the taker uses native ETH instead of WETH to deposit the strike
   if (weth == order.baseAsset && msg.value > 0) {
       // check enough ETH was sent to cover the strike
       require(msg.value == order.strike, "Incorrect ETH amount sent");
   
       // convert ETH to WETH
       // we convert the strike ETH to WETH so that the logic in exercise() works
       // - because exercise() assumes an ERC20 interface on the base asset.
       IWETH(weth).deposit{value: msg.value}();
   } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
       ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
   }

exercise

There are 2 ways to accidentally lose ETH:

1. Exercising a call option with order.baseAsset != weth, exerciser accidentally sends ETH to exercise

   PuttyV2#L422-L443

   if (order.isCall) {
       // -- exercising a call option
   
       // transfer strike from exerciser to putty
       // handle the case where the taker uses native ETH instead of WETH to pay the strike
       if (weth == order.baseAsset && msg.value > 0) {
           // check enough ETH was sent to cover the strike
           require(msg.value == order.strike, "Incorrect ETH amount sent");
   
           // convert ETH to WETH
           // we convert the strike ETH to WETH so that the logic in withdraw() works
           // - because withdraw() assumes an ERC20 interface on the base asset.
           IWETH(weth).deposit{value: msg.value}();
       } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
           ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
       }
   
       // transfer assets from putty to exerciser
       _transferERC20sOut(order.erc20Assets);
       _transferERC721sOut(order.erc721Assets);
       _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
   }

2. Exercising a put option, exerciser accidentally sends ETH to exercise

   PuttyV2#L443-L457

   } else {
       // -- exercising a put option
       // @audit-info there is no check for msg.value == 0, accidentally sent ETH is lost
       // save the floor asset token ids to the short position
       uint256 shortPositionId = uint256(hashOppositeOrder(order));
       positionFloorAssetTokenIds[shortPositionId] = floorAssetTokenIds;
   
       // transfer strike from putty to exerciser
       ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);
   
       // transfer assets from exerciser to putty
       _transferERC20sIn(order.erc20Assets, msg.sender);
       _transferERC721sIn(order.erc721Assets, msg.sender);
       _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
   }

Tools Used

Manual review

Recommended mitigation steps

Add checks to enforce msg.value == 0 where appropriate (see @audit-info annotations).

fillOrder

PuttyV2#L322-L364

// transfer premium to whoever is short from whomever is long
if (order.isLong) {
    ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
} else {
    // handle the case where the user uses native ETH instead of WETH to pay the premium
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the premium
        require(msg.value == order.premium, "Incorrect ETH amount sent");

        // convert ETH to WETH and send premium to maker
        // converting to WETH instead of forwarding native ETH to the maker has two benefits;
        // 1) active market makers will mostly be using WETH not native ETH
        // 2) attack surface for re-entrancy is reduced
        IWETH(weth).deposit{value: msg.value}();
        IWETH(weth).transfer(order.maker, msg.value);
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
    }
}

// filling short put: transfer strike from maker to contract
if (!order.isLong && !order.isCall) {
    ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
    return positionId;
}

// filling long put: transfer strike from taker to contract
if (order.isLong && !order.isCall) {
    // handle the case where the taker uses native ETH instead of WETH to deposit the strike
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the strike
        require(msg.value == order.strike, "Incorrect ETH amount sent");

        // convert ETH to WETH
        // we convert the strike ETH to WETH so that the logic in exercise() works
        // - because exercise() assumes an ERC20 interface on the base asset.
        IWETH(weth).deposit{value: msg.value}();
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
    }

    return positionId;
}

exercise

PuttyV2#L422-L457

if (order.isCall) {
    // -- exercising a call option

    // transfer strike from exerciser to putty
    // handle the case where the taker uses native ETH instead of WETH to pay the strike
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the strike
        require(msg.value == order.strike, "Incorrect ETH amount sent");

        // convert ETH to WETH
        // we convert the strike ETH to WETH so that the logic in withdraw() works
        // - because withdraw() assumes an ERC20 interface on the base asset.
        IWETH(weth).deposit{value: msg.value}();
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
    }

    // transfer assets from putty to exerciser
    _transferERC20sOut(order.erc20Assets);
    _transferERC721sOut(order.erc721Assets);
    _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
} else {
    // -- exercising a put option

    require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

    // save the floor asset token ids to the short position
    uint256 shortPositionId = uint256(hashOppositeOrder(order));
    positionFloorAssetTokenIds[shortPositionId] = floorAssetTokenIds;

    // transfer strike from putty to exerciser
    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

    // transfer assets from exerciser to putty
    _transferERC20sIn(order.erc20Assets, msg.sender);
    _transferERC721sIn(order.erc721Assets, msg.sender);
    _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
}

Lines of code

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

Vulnerability details

Impact

Fees are being sent directly as ERC20 token transfers to the admin/DAO address within PuttyV2.withdraw.

If the fee token transfer reverts, the PuttyV2.withdraw transaction reverts as a whole and assets can not be withdrawn for expired puts or exercised calls.

Proof of Concept

PuttyV2.withdraw

ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);

ERC20 token transfers of fees to the owner() address are subject to reverting token transfers, hence withdrawals are brought to a halt.

Reasons for reverting ERC20 token transfers (see Weird ERC20 Tokens):

• Blacklisted addresses - The owner could theoretically be blocked and prevented from transferring tokens
• ERC777 token is used as order.baseAsset - Receiving owner() is a contract and does not implement the ERC777 function tokensReceived. Transaction reverts

Tools Used

Manual review

Recommended mitigation steps

Consider implementing a pull pattern where fees are not sent directly to the admin/DAO address, instead fees are kept in the PuttyV2 contract and the admin/DAO sweeps (withdraws) fee token balances from time to time.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L303 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L308 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L646-L650

Vulnerability details

Impact

If the maker or taker is a smart-contract that does not implement the onERC721Received method, in the current implementation of fillOrder, the transaction will still be successful and the Putty NFT representing the long/short option position will be minted to the receiver.

However, if the receiver contract is lacking the onERC721Received function, it is not possible to exercise or withdraw due to the usage of safeTransferFrom.

Proof of Concept

Given a long put option for a Bored Ape NFT and a smart-contract acting as the taker but lacking the onERC721Received function:

1. Taker calls fillOrder to fill long put option

2. Long position is minted to maker on L303

   // create long/short position for maker
   _mint(order.maker, uint256(orderHash));

3. Short position is minted to taker on L308

   _mint(msg.sender, positionId);

4. Time passes and the price of the Bored Ape NFT drops. Maker decides to exercise the long put option

   • Maker buys Bored Ape NFT on open market
   • Bored APE NFT is transferred to Putty contract on exercise and maker receives strike in return

5. Taker (smart contract) wants to withdraw the Bored Ape NFT but due to the missing onERC721Received function, _transferERC721sOut (L646-L650) reverts

Taker is not able to withdraw Bored Ape NFT. The asset is stuck in the Putty contract

Tools Used

Manual review

Recommended mitigation steps

Due the usage of safeTransferFrom to transfer NFTs, use _safeMint instead of _mint to prevent improper implemented smart contracts (missing onERC721Received function) from filling orders.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L234-L246 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L498-L501

Vulnerability details

Impact

The admin/DAO can set a fee rate that is applied whenever an option is exercised (as per the function comment on L235).

A taker filling a call option knows the current protocol fee in advance and can estimate the receivable strike (strike - fee) at the time being. However, the fee is subject to change anytime, hence the taker could receive less at the time of withdrawing the strike as initially (at the time of filling the option) anticipated.

Proof of Concept

PuttyV2.setFee

/**
    @notice Sets a new fee rate that is applied on exercise. The
            fee has a precision of 1 decimal. e.g. 1000 = 100%,
            100 = 10%, 1 = 0.1%. Admin/DAO only.
    @param _fee The new fee rate to use.
    */
function setFee(uint256 _fee) public payable onlyOwner {
    require(_fee < 30, "fee must be less than 3%");

    fee = _fee;

    emit NewFee(_fee);
}

PuttyV2.withdraw

if (fee > 0) {
    feeAmount = (order.strike * fee) / 1000; // @audit-info current protocol fee is used. Fee can be different than at the time of the order being filled
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}

Tools Used

Manual review

Recommended mitigation steps

1. Consider making the protocol fee rate a constant (once set it can not be changed)
2. Changes to the protocol fee only apply to options filled after the rate is updated (by storing the current fee rate per order hash within the fillOrder function)

Lines of code

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

Vulnerability details

Impact

Certain ERC-20 tokens do not support zero-value token transfers and revert. Using such a token as a order.baseAsset for a rather small option strike and a low protocol fee rate can lead to rounding down to 0 and prevent asset withdrawals for those positions.

Proof of Concept

PuttyV2.sol#L499-L500

// send the fee to the admin/DAO if fee is greater than 0%
uint256 feeAmount = 0;
if (fee > 0) {
    feeAmount = (order.strike * fee) / 1000;
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); // @audit-info zero-value ERC20 token transfers can revert for certain tokens
}

Some ERC20 tokens revert for zero-value transfers (e.g. LEND). If used as a order.baseAsset and a small strike price, the fee token transfer will revert. Hence, assets and the strike can not be withdrawn and remain locked in the contract.

See Weird ERC20 Tokens - Revert on Zero Value Transfers

Example:

• order.baseAsset is one of those weird ERC-20 tokens
• order.strike = 999 (depending on the token decimals, a very small option position)
• fee = 1 (0.1%)

$((999 * 1) / 1000 = 0.999)$ rounded down to 0 -> zero-value transfer reverting transaction

Tools Used

Manual review

Recommended mitigation steps

Add a simple check for zero-value token transfers:

// send the fee to the admin/DAO if fee is greater than 0%
uint256 feeAmount = 0;
if (fee > 0) {
    feeAmount = (order.strike * fee) / 1000;

    if (feeAmount > 0) {
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }
}