code423n4 commented 2 years ago

Lines of code

Vulnerability details

Impact

Detailed description of the impact of this finding.

   function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check user owns the position
        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

        // check position is long
        require(order.isLong, "Can only exercise long positions");

        // check position has not expired
        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");

        // check floor asset token ids length is 0 unless the position type is put
        !order.isCall
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

        /* ~~~ EFFECTS ~~~ */

        // send the long position to 0xdead.
        // instead of doing a standard burn by sending to 0x000...000, sending
        // to 0xdead ensures that the same position id cannot be minted again.
        transferFrom(msg.sender, address(0xdead), uint256(orderHash));

        // mark the position as exercised
        exercisedPositions[uint256(orderHash)] = true;

        emit ExercisedOrder(orderHash, floorAssetTokenIds, order);

        /* ~~~ INTERACTIONS ~~~ */

        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 {
                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

            // 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);
        }
    }

contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Ownable {
    /* ~~~ TYPES ~~~ */

    using SafeTransferLib for ERC20;

When call exercise() function, solmate’s is used for pulling from the caller’s account, this issue won’t exist if OpenZeppelin’s SafeERC20 is used instead.SafeTransferLibvault.token

That’s because there is a subtle difference between the implementation of solmate’s and OZ’s :SafeTransferLibSafeERC20

OZ’s checks if the token is a contract or not, solmate’s does not.SafeERC20SafeTransferLib

See: https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As a result, when the token’s address has no code, the transaction will just succeed with no error.

This attack vector was made well-known by the qBridge hack back in Jan 2022.

Since the fillOrder() function uses

require(order.baseAsset.code.length > 0, "baseAsset is not contract");

to check the length of the contract code, the fillOrder function does not have the problem, but other functions have the The problem.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Consider using OZ’s instead.SafeERC20

kirk-baird commented 2 years ago

Code size is checked in Putty for ERC20 transfers https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L293 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598

outdoteth commented 2 years ago

confirming what @kirk-baird said

HickupHH3 commented 2 years ago

dup of #299

code-423n4 / 2022-06-putty-findings