Putty contest - antonttc's results

Lines of code

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

Vulnerability details

Impact

Admin can cause significant loss for option sellers compared to premium

Proof of Concept

it’s possible for seller to suffer a net loss even if the option doesn’t got exercised because of the admin power of setting fee. There is also no check on whether strike * fee is smaller or greater than the premium the seller get, which means sellers can accidentally put an order that is absolutely not worth it.

e.g.: I sell a 100ETH put on an NFT for 3 days with premium of 3 eth, After expiry and before taking out the 100 ETH, owner change the fee rate to maximum (3%), i got charge another 3% which is my premium, ended in an absolute net loss that I didn’t want to agree on.

Recommended Mitigation Steps

it’s better to embed a “fee” parameter in the order structure, and when filling order, make sure order.fee == fee, so no one can avoid paying fee after the protocol turn the switch on, and then check premium > strike * fee to make sure sellers are not doing something stupid.

while withdrawing, use the fee in the order struct to make sure this rate is agreed by both parties when filling the order. This should remove all concerns regarding fees.

Low risk findings

1. filling orders with msg.value doesn’t work with batchFillOrder

in fillOrder while filling a long order the function check msg.value == order.strike to move collateral to the contract, or check msg.value == premium and pay the seller directly. This will not work with batchFill because msg.value will be a the same value across all iterations. So if user want to fill 2 short order both with weth as premium at the same time, fillOder will fail

Mitigation:

Consider refactor the fillOrder and batchFillOrder into the following:

function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
		_fillOrder(order, signature, floorAssetTokenIds, mgs.value)
}

function batchFillOrder(
        Order[] memory orders,
        bytes[] calldata signatures,
        uint256[][] memory floorAssetTokenIds
        uint256[] calldata values
    ) public returns (uint256[] memory positionIds) {
        require(orders.length == signatures.length, "Length mismatch in input");
        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");

        positionIds = new uint256[](orders.length);

        uint256 totalValue;
        for (uint256 i = 0; i < orders.length; i++) {
            positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i], values[i]);
            totalValue += value[i]
        }

        require(totalValue == msg.value, "value mismatch");
    }

// internal _fillOrder
function _fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
        uint256 _value;
    ) public payable returns (uint256 positionId) {
		...
    {use _value instead of msg.value}
    ...
}

2.order.nonce doesn’t work as its name suggested

Usually the word nonce refers to a increasing number that can be used as a way to “cancel old orders”. but in this case, nonce is purely used as “salt” to produce different id for different orders. I think it’s better that either rename this to salt , or add a check of order.nonce>last_ nonce in order validation step.

3. exercise can be marked as exeternal