Putty contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L20-L37 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L259-L380

Vulnerability details

Impact

by calling fillOrder() contract mint two NFT token position for Long and Short position for maker and taker and also code transfers premium (which is based on order.baseAsset) from Long position owner to Short position owner and transfers collaterals from Short position owner to PuttyV2 address so Long positions owner can claim those collaterals by calling exercise() and paying the strike price. all the address of tokens are defined in order object so Attacker can sign malicious order which look profitable but transfers NFT position token of caller to attacker (set NFT position token of caller as premium or collateral, the ID of caller NFT position token is calculable when attacker signs the order) and if any user tries to fill that order he will lose his funds. so the impact of this bug is that users would lose their funds by filling orders.

Proof of Concept

To exploit this attacker have multiple choices, the real bug is that contract allows token address in order to be equal to PuttyV2 address, it means attacker can create and sign an order which look very good for filler but when someone calls fillOrder() for that order, code would mint caller an NFT position token but transfer that NFT to attacker as premium or to contract as collateral(which attacker can claim later) and user would lose his funds. the detail steps of exploit is:

1. attacker would sign this order (order.isLong=False & order.baseAsset=PuttyV2 & order.premium=oppositeOrderHash & <set other parameters to make filling order very profitable for filler and to transfer caller tokens as collateral >)
2. attacker would give all token spending allowance to PuttyV2 address.
3. user would see attacker order and as it is looks very profitable he would call fillOrder() for that order. (attacker can somehow trick users to click on fillOrder() too by creating a lot of order)
4. fillOrder logic would mint NFT positions for caller but in the line 338 (ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium)) code would transfer NFT position token of caller to the attacker address because: (order.baseAsset == PuttyV2 and msg.sender == user and order.maker == attacker and order.premium == caller NFT position token ID) and also code would transfer other user tokens that are specified in order as collateral.
5. users funds would be transferred into contract but user didn't receive any NFT position token and the NFT position token of Long and Short position would be in attacker address. (attacker can create PUT option and transfer user funds as collateral) and attacker would receive both Long and Short NFT position token.

The other thing that attacker do is in this steps:

1. attacker would sign this order: (order.isLong=True & order.isCall=True & order.erc721Assets=(address=PuttyV2, id=oppositeOrderHash) & <set other parameters to make filling order very profitable for filler and to transfer caller tokens as collateral>)
2. attacker would give all token spending allowance to PuttyV2 address.
3. user would see attacker order and as it is looks very profitable he would call fillOrder() for that order. (attacker can somehow trick users to click on fillOrder() too by creating a lot of order)
4. fillOrder logic would mint NFT positions for caller but in the line 376 (_transferERC721sIn(order.erc721Assets, msg.sender);) code would transfer NFT position token of caller to the PuttyV2 address because: (order.erc721Assets == [(address=PuttyV2, id= caller NFT token ID)] and msg.sender == user) and also code would transfer other user tokens that are specified in order as collateral.
5. users funds would be transferred into contract as collateral but user didn't receive any NFT position token and the NFT position token of Long and Short position would be in attacker address. attacker would call exercise() and withdraw() to get his and user tokens from contract.

So because contract logics allows token addresses in order to be PuttyV2 address it's possible to create malicious order that it seems very profitable for caller but in fact it transfers user NFT position token to contract as collateral or to attacker as premium so user couldn't use the filled order position after calling fillOrder() and his funds would be lost.

Tools Used

VIM

Recommended Mitigation Steps

don't allow the token address in order to be PuttyV2 address.

Lines of code

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

Vulnerability details

Impact

It's possible to steal all the token (ERC20 and ERC721) balance of PuttyV2 in one transaction and spend them and then return them (like flash-loan) and pay no fee at all. This bug has two Impact, first one can take ownership of NFT tokens for one transaction which can impact reputation of user NFT tokes, secondly attacker can use all balance of contract as flash loan without paying any fee, but flash-loan in lending protocols has 0.3% fee at least. So it's possible to abuse users collateral in contract.

Proof of Concept

This is fillOrder() code:

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check signature is valid using EIP-712
        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

        // check floor asset token ids length is 0 unless the order type is call and side is long
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

        /*  ~~~ EFFECTS ~~~ */

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

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

        // save floorAssetTokenIds if filling a long call order
        if (order.isLong && order.isCall) {
            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;
        }

        // save the long position expiration
        positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;

        emit FilledOrder(orderHash, floorAssetTokenIds, order);

        /* ~~~ INTERACTIONS ~~~ */

        // 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 {
                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 {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

            return positionId;
        }

        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }
    }

As you can see in the end of the function contracts tries to transfer ERC20 and ERC721 which are collateral from msg.sender or order.maker to contract address(by calling _transferERC20sIn() and _transferERC721sIn() with tokens defined in order object). code makes external calls to transfer collatral tokens to contract address. attacker can use this external calls and make contract to call attacker controlled address and then reenter the contract when the external call happens. when this calls happens PuttyV2 NFT position token is already minted for maker and taker so attacker can call exercise(). The summary of exploitation is that attacker would fill his own specially crafted order (he needs multiple malicious contracts). the collateral of the order is all PuttyV2 balances, but before PuttyV2 tries to transfer collaterals from attacker, attacker can call exercise() in one of fillOrder() external calls and receive all the collaterals(all PuttyV2 balances) and then use them as free flash loan then let PuttyV2 transfers them to contract as collateral.

these are the detail steps of this exploit:

1. attacker creates a contract named makerAttcker to sign order. (it's possible to sign orders with contract because SignatureChecker.isValidSignatureNow() supports it, and in reality it's just a external call to the singer address to check signature)
2. attacker creates a malicious ERC20 contract named erc20Attacker
3. attacker signs this order with makerAttacker : attackerOrder = Order(Long, Call, baseAsset=erc20Attacker, erc20Assets=[list of all PuttyV2 erc20 balances], erc721Assets=[list of all PuttyV2 erc721 balances])
4. attacker would make transaction(with walletAttacker address) and call fillOrder() with attackerOrder data. fillOrder logic will mint Long NFT token for makerAttacker. also attacker gave full spending allowance to PuttyV2 contract address.
5. fillOrder() execution in the line 324 (ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium)) tries to transfer premium from maker to taker address, but because order.baseAsset is equal to erc20Attacker which is attacker controlled contract so PuttyV2 would make a external call to attacker controlled contract and the execution would be in attacker hand.
6. attacker erc20Attacker contract would call makerAttacker contract and makerAttacker contract would call PuttyV2.exercise(attackerOrder).
7. exercise() code would checks the conditions and they will pass and then in lines 422 to 442 it would tries to transfer collaterals to msg.sender which is makerAttacker contract. (PuttyV2 would call order.baseAsset too which is erc20Attacker address and it returns without doing anything) because collaterals are erc20Assets=[list of all PuttyV2 erc20 balances], erc721Assets=[list of all PuttyV2 erc721 balances] which basically are all token balance of PuttyV2 so all the balances of protocol would get tranferred to makerAttacker.
8. contract makerAttacker would receive tokens from PuttyV2 and then execution would return to makerAttacker and it will perform all the action attacker needs with ERC20 and ERC721 tokens as free flash-loan and then transfers those tokens to walletAttacker address.
9. then execution of makerAttacker would be finished and execution would get back to erc20Attacker and it would return and then the execution would reach to fillOrder() in lines 373 to 378 and code would transfers all the collateral to contract address from walletAttacker. collaterals are all the PuttyV2 tokens which in the last steps get transferred to walletAttacker address.

So in general attacker was able to get free flash-loan of all PuttyV2 balances which has multiple critical impact for users tokens and protocol publicity.

Tools Used

VIM

Recommended Mitigation Steps

have some mechanism to prevent reentrancy.

Lines of code

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

Vulnerability details

Impact

In fillOrder() code uses mint() to create NFT token for maker and taker and if those addresses were for other smart contract and didn't implement NFT token operations, then PuttyV2 NFT tokens could be locked in those addresses and funds wouldn't be accessible. and there are multiple cases that maker or taker could be a smart contract and by using mint() fund could be locked in those addresses if they couldn't handle ERC721 tokens.

Proof of Concept

This is fillOrder() code:

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check signature is valid using EIP-712
        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

        // check floor asset token ids length is 0 unless the order type is call and side is long
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

        /*  ~~~ EFFECTS ~~~ */

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

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

        // save floorAssetTokenIds if filling a long call order
        if (order.isLong && order.isCall) {
            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;
        }

        // save the long position expiration
        positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;

        emit FilledOrder(orderHash, floorAssetTokenIds, order);

        /* ~~~ INTERACTIONS ~~~ */

        // 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 {
                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 {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

            return positionId;
        }

        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }
    }

As you can see it uses mint() for creating NFT tokens which can cause tokens to be locked in addresses that can't handle ERC721 tokens. SafeMint() is safe because it checks whether the receiver can receive ERC721 tokens. That can prevent the case that a NFT will be minted to a contract that cannot handle ERC721 tokens. filleOrder() function don't use safeMint(). There are multiple scenarios that maker and taker could be contract addresses.

1. maker could be a smart contract, in this case, the signature of order is validated against that smart contract using ERC1271.
2. taker could be a smart contract by sending signed orders. to support safely smart contract addresses when they are maker or taker code should use safeMint() to create NFT tokens. (also prevent any possible reentranacy attack)

Tools Used

VIM

Recommended Mitigation Steps

use safeMint() instead of mint()

Lines of code

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

Vulnerability details

Impact

Owner can change the value of contract parameters like fee immediately, this isn't the best practice and it can cause some security issue for users as owner can change fee and then users orders would get filled based on new fee value even so when user was signing the orders they signed based on different fee value(their strategy was for another fee value and they didn't get any notice about fee value changes).

Proof of Concept

This is setBaseURI() and setFee() code:

    /**
        @notice Sets a new baseURI that is used in the construction
                of the tokenURI for each NFT position. Admin/DAO only.
        @param _baseURI The new base URI to use.
     */
    function setBaseURI(string memory _baseURI) public payable onlyOwner {
        baseURI = _baseURI;

        emit NewBaseURI(_baseURI);
    }

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

As you can see there is no proposal + time-lock mechanism for this admin operations. admin can change fee and users don't get any notice for that and users order would fill with this new fee even so they signed the orders when contract fee was different. for example a user could perform high frequency trading when fee is low and he could have a lot of orders signed and his strategy is profitable for him while fee is very low, but suddenly when fee changes to 30pip then other users could fill all of the orders or that user and that user would lose a lot of fund because he didn't have any chance to cancel his orders. same scenario is valid for setBaseURI(), if some one has some logic based on that then they don't get any verified notice about changing the value.

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to prevent admin sudden changes.

Lines of code

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

Vulnerability details

Impact

order.duration is supposed to show the duration that option is valid, but code allows it to be 0, so anyone who take long position with order.duration==0 is going to lose premium without having any chance of exercising that order. positionExpirations[] shows expiration time of orders but the condition in the code for checking that order is expired or not is strict in exercise() and withdraw() so when block.timestamp equals positionExpirations[orderHash] it's not possible to call exercise() or withdraw() and contract logic won't work for that order in the last second. for order.duration==0 case positionExpirationsp would be the same block timestamp that order filled and order would fill but it wouldn't be possible to exercise() that order and the one who took Long position would lose premium.

Proof of Concept

This is fillOrder() code:

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check signature is valid using EIP-712
        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

        // check floor asset token ids length is 0 unless the order type is call and side is long
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

        /*  ~~~ EFFECTS ~~~ */

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

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

        // save floorAssetTokenIds if filling a long call order
        if (order.isLong && order.isCall) {
            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;
        }

        // save the long position expiration
        positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;

        emit FilledOrder(orderHash, floorAssetTokenIds, order);

As you can see it allows filling orders with order.duration == 0 and it sets expiration time as: positionExpirations[] = block.timestamp + order.duration. This is exercise() code:

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

As you can see it uses this line: require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired"); to check that position is not expired. and This is withdraw() code:

    function withdraw(Order memory order) public {
        /* ~~~ CHECKS ~~~ */

        // check order is short
        require(!order.isLong, "Must be short position");

        bytes32 orderHash = hashOrder(order);

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

        uint256 longPositionId = uint256(hashOppositeOrder(order));
        bool isExercised = exercisedPositions[longPositionId];

        // check long position has either been exercised or is expired
        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

As you can see it uses this line require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired"); to check to see that if position is expired. Order expiration time is set on fillOrder() and it is checked on exercise() and withdraw() function. In exercise() for checking that orders are not expired, code uses require(block.timestamp < positionExpirations[uint256(orderHash)]) and the condition in withdraw() to check that order is expired is require(block.timestamp > positionExpirations[longPositionId], so when block.timestamp equals to positionExpirationsp[orderHash] those two checks would fail and it wouldn't be possible to perform exercise() and withdraw and for that second contract logic would not work.

So if anyone creates or fills an long position with order duration equal to 0 then s/he is going to lose the premium, because contract allows for those orders to be filled but it doesn't allow them to be exercised, so attacker can create tons of short positions with duration equal to 0 and very preferable for long side(for example put option for BTC at strike price $100K and premium $1K), and if users tries to fill and exercise those orders (get tricked to click on fill and exercise) they would lose the premuim.

And for any contract or code that works with PuttyV2 when block.timestamp==positionExpirations[orderHash] it's not possible to call exercise() or withdraw(). in general in that second user should be able to either all exercise() or withdraw() for that position, but with current logic it's not possible to do it. This bug is preventing one contract to fill and exercise orders in the same block when order.duration is equal to 0 too.

Tools Used

VIM

Recommended Mitigation Steps

prevent orders with order.duration equal to 0 to be filled and also allow one of exercise() or withdraw() when block.timestamp==positionExpirations[orderHash]

Lines of code

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

Vulnerability details

Impact

It's common practice in EIP-712 encoding to use nonce to identify singer messages and prevent replay attacks and create ability to make signed messages invalid. but in PuttyV2 uses order hash for identifying orders and cancelling them. this can cause some problems like:

1. duplicate nonce for different orders
2. can't quickly identify cancelled orders without order's all info
3. can't cancel multiple orders
4. can't cancel order without knowing order's all parameters This problems and similar problems are created because order hash is not iterable and also to calculate order hash it requires all order info. But nonce can be iterable and only the nonce of orders are required to identify orders.

Proof of Concept

This is part of filleOrder() code:

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check signature is valid using EIP-712
        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

        // check floor asset token ids length is 0 unless the order type is call and side is long
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

        /*  ~~~ EFFECTS ~~~ */

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

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

and this is cancel() code:

    function cancel(Order memory order) public {
        require(msg.sender == order.maker, "Not your order");

        bytes32 orderHash = hashOrder(order);

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

        emit CancelledOrder(orderHash, order);
    }

As you can see code uses hashOrder() to create orderHash and uses orderHash to identify and mark orders. but this create some problems because order hash is not iterable and also to calculate an order identity all the order info is required. it's a best practice to use nonce in EIP-712 to identify one signer's messages and prevent replay attack and also mark the signed messages. some problems with using order hash is:

1. duplicate nonce for different orders it's possible to use same nonce for different orders, this can confuse other projects that are working with PuttyV2 because it's common practice that signed messages shouldn't have same nonce.
2. can't quickly identify cancelled orders without order's all info by looking at contract storage state it's not possible to quickly identify which orders are cancelled and to identify cancelled orders it requires to collect all the cancelled orders information to identify cancelled orders. to check that if is an order is cancelled it requires to know all the order information.
3. can't cancel multiple orders it isn't possible for user to cancel multiple orders, This is can be a serious problem when user has multiple signed orders and market crashes (line luna) and user wants to cancel all of them but right now user need to cancel them one by one.
4. can't cancel order without knowing order's all parameters if a user loses his signed order info, then he can't cancel them. orders data are stored off-chain and it's possible to lose them, so in case of some accidents for off-chain data user can't cancel his orders. this is also a problem for smart contracts that are going to work with PuttyV2, they need to store all the order information on-chain to be able to cancel them.

to prevent all this issues it's best practice to use (signer, nonce) pair to identify orders and mark them. for example there could be a map signer-> minValidNonce that user specify minimum valid nonce for his orders and with that user can cancel all his current orders without knowing any information about them.

Tools Used

VIM

Recommended Mitigation Steps

use nonce for identifying orders and use order hash and signature for validating orders. also code needs to save map (singer, nonce)->orderHash to validate orders later.

Lines of code

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

Vulnerability details

Impact

The exact impact of this issue is that contract storage data would be in a wrong state and if other contract or tools use this data for some logics their logics would work incorrectly because it's possible to set orders as cancelled after they are filled and exercised and if one looks at the contract storage values for that orders it will show that order is filled and cancelled in the same time.

Proof of Concept

This is cancell() code:

    /**
        @notice Cancels an order which prevents it from being filled in the future.
        @param order The order to cancel.
     */
    function cancel(Order memory order) public {
        require(msg.sender == order.maker, "Not your order");

        bytes32 orderHash = hashOrder(order);

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

        emit CancelledOrder(orderHash, order);
    }

As you can see there is no check that order is filled or not and code sets the order status as cancelled and if order has been filled and exercised before then order have NFT position and exercisedPositions and cancelledOrders as true.

Tools Used

VIM

Recommended Mitigation Steps

check order NFT token position existence before setting cancelling status.