Putty contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

If the baseAsset is not WETH and the taker mistakenly sends ETH, this value will be lost. Given the checks performed in the if above this else clause, it seems natural that the contract should check that msg.value == 0.

Tools Used

Manual review.

Recommended Mitigation Steps

In the case where the baseAsset is not WETH, add the following require statement to the else block.

            } else {
                require(msg.value == 0),
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

QA Report

[L-01] Remove payable modifiers

While the payable modifier may save a miniscule amount of gas, functions that are not intended to receive ETH should revert if msg.value is sent.

[L-02] Timecheck should be inclusive

Given that the fillOrder() and exercise() have non-inclusive time checks, withdraw() should include the expiration time. Otherwise, there will be a gap.

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

[L-03] Use safeTransfer()

Using transfer() on the WETH contract is pretty safe, but it's still worth using safeTransfer() consistently throughout the contract.

[L-04] Code vs comment conflict

The order of filling and cancelling the orders occurs differently in the code than in the comment.

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

Gas Report

[G-01] For loop optimization

The following for loops can be optimized by incrementing i in an unchecked block.

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