code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L422-L442 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L293 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L598

Vulnerability details

Impact

It is safe to check order.baseAsset.code.length and token.code.length in fillOrder() and _transferERC20sIn. Because SafeTransferLib doesn’t check whether the erc20 token is actually a contract.

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

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

However, order.baseAsset.code.length is not checked in exercise(). Therefore, if the base token is destroyed after filling the order. The call option exerciser can still get the asset without transferring any token.

Proof of Concept

Both fillOrder() and _transferERC20sIn checked the code length of token

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L293 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L598

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        …
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
        …
    }

    function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
        for (uint256 i = 0; i < assets.length; i++) {
            …
            require(token.code.length > 0, "ERC20: Token is not contract");
            require(tokenAmount > 0, "ERC20: Amount too small");
            …
        }
    }

But exercise() doesn’t check the code length of order.baseAsset. If order.baseAsset is destroyed after filling the order. The call option exerciser can still successfully get the asset without transferring any token. Because SafeTransferLib won’t check whether the token is actually a contract before doing any transfers.

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L422-L442

    function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
        …

        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)]);
        }
        …

Tools Used

None

Recommended Mitigation Steps

There are two methods to fix this problem.

The first one is adding order.baseAsset.code.length check in exercise

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

The second method is using OpenZelppelin's SafeTransfer instead of SafeTransferLib.

outdoteth commented 2 years ago

there is a codesize check here: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L293

HickupHH3 commented 2 years ago

Dup of #299; agree with sponsor.

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