Putty contest - 0xsanson's results

Lines of code

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

Vulnerability details

Impact

The protocol takes a fee proportional to the order's strike. This happens during a withdraw:

// transfer strike to owner if put is expired or call is exercised
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    if (fee > 0) {
        feeAmount = (order.strike * fee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

    return;
}

It doesn't make sense to take a fee proportional to the strike when the order is unexercised. This fee will make some orders unprofitable for both sides.

Proof of Concept

Alice makes a put order with premium 100$ and strike 10000$. The current fee is 2%. When Bob fills the order this happens: (i) Alice pays Bob 100$, (ii) Bob deposits 10000$. If the option doesn't get exercised, Bob will call withdraw getting 10000*(100-2)=9800$ back. So at the end, both Alice and Bob end up losing 100$ each, and the owner gets 200$.

Clearly, in this situation it would be expected for Bob to win something.

Recommended Mitigation Steps

Since an option can end up unexercised and it doesn't make sense to collect a fee on the strike, the solution is taking a fee on the premium during fillOrder. The fee on the strike should be collected only if the order was exercised.

Judge has assessed an item in Issue #417 as Medium risk. The relevant finding follows:

(L1) fee can change for ongoing orders

The owner can call setFee(uint256 _fee) and change the fee amount. This changes the fee taken for all orders already filled/exercised.

In some situations, an user may not have filled an order if they knew the fee would end up higher. This situation is alleviated by the fact that the fee is capped at 3%.

Recommendations

The fee can be written in the Order struct and checked that it matches the current correct value during fillOrder. This way when exercising/withdrawing we can use the "order.fee" irrespective to the current global variable.

(L2) fee only for call orders

The protocol collects a fee proportional to the strike. In the current implementation, this happens only during the withdraw of a exercised call order (or unexercised put) (lines). I think there should be a fee also for the other order, but it got missing.

Recommendations

Add a similar fee collection during exercise of a put order. Overall, check all possible paths and look if the fees collected make sense.

(L3) Some functions are payable even though they don't accept ether

Functions setBaseURI and setFee are payable but they don't use ether. If the owner sets msg.value > 0 by mistake, it will get lost.

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

Recommendations

Remove the payable keyword in these functions.

(L4) Positions NFT mints aren't "safe"

During a fill, two ERC721 tokens are minted (lines):

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

If the recipient is a contract, it may misbehave and lose the NFT, since it may expect a onERC721Received call.

Since order.maker can't be a contract (we need its signature), this only applies to msg.sender.

Recommendations

Document that NFTs are minted this way, so contracts interacting with Putty can be aware. _safeMint is not suggested so we can save gas.

(N1) Magic value can be saved as constant

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

The value 10_000 days can be saved as uint256 private constant MAX_DURATION. This helps the reader keeping track of values.

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

(N2) Order can be cancelled multiple times

The function cancel(Order memory order) can be called multiple times for the same order. No big problem, the only issue here is that it continues emitting a CancelledOrder, which may lead to unexpected behavior for off-chain software. Suggested adding:

require(!cancelledOrders[orderHash], "Order already cancelled");

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

(G1) Array length is read multiple times

In multiple instances, the same array length is read multiple time from memory/calldata. It would be better to save the value on the stack once and for all. Example:

    function batchFillOrder(
        Order[] memory orders,
        bytes[] calldata signatures,
        uint256[][] memory floorAssetTokenIds
    ) 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);

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

It can be rewritten like:

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

        positionIds = new uint256[](_length);

        for (uint256 i = 0; i < _length; i++) {
            positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]);
        }
    }

Lines

grep -r '\.length'

(G2) Error codes instead of strings

Recent versions of solidity support custom errors. They are more gas efficient than error strings when deploying the contract and when the error is raised. For example:

require(msg.sender == order.maker, "Not your order");

It can be rewritten like:

error NotYourOrder(); // this in the global scope
...
if(msg.sender != order.maker) revert NotYourOrder();

Reference

Lines

grep -r 'require'

(G3) Transferring ERC721 IN doesn't need to be "safe"

function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
    for (uint256 i = 0; i < assets.length; i++) {
        ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);
    }
}

Here safeTransferFrom performs a onERC721Received call to address(this), which isn't needed (we know it does nothing).

Suggested using the simpler transferFrom.

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

(G4) Opposite order creates unnecessary memory

    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
        // use decode/encode to get a copy instead of reference
        Order memory oppositeOrder = abi.decode(abi.encode(order), (Order));

        // get the opposite side of the order (short/long)
        oppositeOrder.isLong = !order.isLong;
        orderHash = hashOrder(oppositeOrder);
    }

Here oppositeOrder is created by copying order to new memory. It would be better working with the same memory reference. We can make it safe by recasting order.isLong at the end.

    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
        bool _isLong = order.isLong;   // save on stack

        order.isLong = !_isLong;
        orderHash = hashOrder(order);

        order.isLong = _isLong;
    }

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

(G5) for-loop i++

In general, a for-loop like this can be made more gas efficient.

for (uint256 i = 0; i < length; i++) {
    ...
}

Consider rewriting it like this:

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Lines

grep -r 'i++'