Putty contest - shenwilly's results

Lines of code

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

Vulnerability details

If fillOrder is called by a contract that expect onERC721Received to be called when receiving NFT, it could potentially result in irregular state for the receiver contract.

PuttyV2.sol#L303-308

_mint(order.maker, uint256(orderHash));
...
_mint(msg.sender, positionId);

The current implementation uses _mint instead of _safeMint. As ERC721 transfers within the contract use the safeTransferFrom variant, it would be better to keep it consistent and use the safe variant, so as not to confuse contract developers that integrate Putty.

Proof of Concept

• Alice wants to make a contract that could interact with Putty.
• Alice expects that Putty conforms to ERC721TokenReceiver standard after seeing the ERC721.safeTransferFrom implementation, so she relies on onERC721Received to update the contract's state when minting the option NFT.
• Alice tried to fulfill an order via the contract and lost her funds as the contract's state is not updated properly after mint, making it unable to call withdraw or exercise.

Recommended Mitigation Steps

Add _safeMint function in PuttyV2Nft.sol and use it instead of _mint in fillOrder().

Lines of code

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

Vulnerability details

Some ERC20 tokens do not allow zero-value transfer and would revert on transfer. In the current implementation of withdraw, it's possible for the feeAmount to return zero but still attempting to transfer, causing users inability to withdraw funds when using such tokens as the baseAsset.

PuttyV2.sol#L497-L501

uint256 feeAmount = 0;
if (fee > 0) {
    feeAmount = (order.strike * fee) / 1000;
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}

When order.strike * fee is lower than 1000, feeAmount will be 0.

Recommended Mitigation Steps

Put the zero value check on feeAmount instead of fee:

uint256 feeAmount = (order.strike * fee) / 1000;
if (feeAmount > 0) {
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}

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

Low Risk Vulnerabilities

1. Missing sanity check on duration

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

There is no minimum order.duration check. A faulty frontend or user mistake could input a very low duration, causing the option to expire near instantly.

This will allow short order takers to receive the premium without taking the risk, as the funds can be withdrawn immediately.

Recommended Mitigation Steps

Consider adding a minimum duration check:

require(order.duration >= 3600, "Duration too short");

2. Inclusive fee limit

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

require(_fee < 30, "fee must be less than 3%");

This check conflicts with the unit test testItCannotSetFeeGreaterThan3Percent, which tests that fee should not be greater than 3%, instead of lower than 3%.

Recommended Mitigation Steps

It's more intuitive to use must not be greater than check. Consider updating the code to use the inclusive comparison operator instead:

require(_fee <= 30, "fee must not be greater than 3%");