Putty contest - MadWookie's results

QA

Functions never used internally should be declared external

1. File: PuttyV2.sol#L389

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

Magic numbers should be declared as constants

1. File: PuttyV2.sol#L287

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

2. File: PuttyV2.sol#L499

feeAmount = (order.strike * fee) / 1000;

Functions setBaseURI() and setFee() can lock Ether in contract forever.

There is no way to remove Ether from this contract if accidentally sent to these functions.

1. File: PuttyV2.sol#L228-232

function setBaseURI(string memory _baseURI) public payable onlyOwner {
    baseURI = _baseURI;


    emit NewBaseURI(_baseURI);
}

2. File: PuttyV2.sol#L240

function setFee(uint256 _fee) public payable onlyOwner {
    require(_fee < 30, "fee must be less than 3%");


    fee = _fee;


    emit NewFee(_fee);
}

GAS

unckecked{} and ++i can be used in for-loops to reduce gas cost of execution and deployment cost.

This does slightly reduced code readability but could be beneficial if large arrays are expected to be used.

Example

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

    unchecked {
        ++i;
    }
}

Locations where this can be implemented

1. File: PuttyV2.sol#L556

2. File: PuttyV2.sol#L598

3. File: PuttyV2.sol#L615

4. File: PuttyV2.sol#L641

5. File: PuttyV2.sol#L651

6. File: PuttyV2.sol#L662

7. File: PuttyV2.sol#L674

8. File: PuttyV2.sol#L732

9. File: PuttyV2.sol#L746

Ordering require() statements from least gas to most gas when possible will make failing cheaper on average.

These four require statements can be placed at the start of fillorder()

1. File: PuttyV2.sol#L275-293

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

like so

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

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

The same can be done for the below require statements in exercise()

2. File: PuttyV2.sol#L398-406

// check position is long
require(order.isLong, "Can only exercise long positions");


// check floor asset token ids length is 0 unless the position type is put
!order.isCall
    ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
    : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");