Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L289-L291 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L315-L316 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L400-L401 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L480-L481

Vulnerability details

Impact

for checking order expiration or position expiration and setting the expiration time code uses block.timestamp but miners can control block.timestamp to some extend. miners can manipulate block.timestamp whenever they see fit and cause other user's transactions to be fail and cause griefing. This can prevent low duration orders to to totally fail, because miners can easily prevent user's calls to exercise(), withdraw() or fillOrder() by changing block.timestamp. also miners can DOS user's transactions near order expire time or positions expire time. using block.timestamp in logics totally creates MEV and miners can manipulate and cause fund loss in some scenarios, which users didn't expect them based contract logic.

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 uses block.timestamp < order.expiration to see that if order is expired and it sets position expiration time as: positionExpirations[order.isLong ? uint256(orderHash) : positionId] = 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 block.timestamp < positionExpirations[uint256(orderHash)] to check that if order is still valid or expired. 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 code uses block.timestamp > positionExpirations[longPositionId] to see that if position is expired or not.

So miners can change block.timestamp and cause transaction which are calls to fillOrder(), exercise() and withdraw() to fail and prevent users from benefiting from their positions and cause griefing and fund loss. This issue is critical in high frequency orders and when order's durations are low and miners can totally damage this type of orders, because this orders have low duration so miner can control which calls about this orders fail for most of positions duration. for other orders miners can cause unexpected behavior for users, for example when users tries to call exercise() in last seconds miner can cause this transaction to fail by manipulating block.timestamp and cause users to lose funds. in some cases like sudden price crash of some coins (like what happened to LUNA) users would lose lots of money if they couldn't exercise() or withdraw()

Tools Used

VIM

Recommended Mitigation Steps

Use block.number for expiration time or don't allow low duration orders and inform users about this attack vector.

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