Putty contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

A user might have 2 filled orders unexpectedly by canceling an already filled order. Then she might pay the premium 2 times even though she wanted only one order.

Proof of Concept

Currently, it doesn't check whether the order is filled or not when a user cancels her order. So a user might miss the FilledOrder event and fill another order after canceling the old order.

1. Alice opened a long put order.
2. 10 mins later, while checking active orders, she finds a counter offer and fills the order using acceptCounterOffer().
3. Btw her original order was filled 5 mins ago and she missed the event, unfortunately.
4. Inside the acceptCounterOffer(), her filled order is considered canceled and she has 2 long positions now even though she wanted only one position.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should check whether the order was filled already when canceling an order so that users don't confuse filled and canceled orders. We can modify cancel() like below.

function cancel(Order memory order) public {
    require(msg.sender == order.maker, "Not your order");

    bytes32 orderHash = hashOrder(order);

    require(ownerOf(uint256(orderHash)) == address(0), "Filled already");

    // mark the order as cancelled
    cancelledOrders[orderHash] = true;

    emit CancelledOrder(orderHash, order);
}

Lines of code

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

Vulnerability details

Impact

An order maker might steal premium from the sender by setting order.strike = 0. It's known that some ERC20 tokens don't allow 0 transfer and it will revert when the order sender tries to exercise his long position. So the order maker will receive back his tokens after the order is expired.

Proof of Concept

As we can see here, some tokens don't allow 0 transfer. Let's call the token TOK and assume 1 TOK = 1 USDC. And I can't find a restriction on that order.strike must be positive and I think it would be possible to create an order that has a positive premium and zero strikes.

1. Alice creates a short call order with the below settings. premium = 5000 TOK, strike = 0 TOK, ERC20 token = 10000 USDC
2. Because this order is profitable for senders, Bob fills this order by sending 5000 TOK premium.
3. After that, Bob tries to exercise the order but will revert because of this one.
4. After the order is expired, Alice withdraws her 10000 USDC.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should check order.strike != 0 before transfer. Modification for L436.

if(order.strike != 0) {
    ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
}

In the fillOrder() function, there are similar transfers here and here, but there will be no loss of funds because the order won't be filled anyway.

Lines of code

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

Vulnerability details

Impact

An attacker(order maker) might make the sender can't withdraw tokens using vulnerable tokens. So the sender can't receive tokens after strike payment.

Proof of Concept

I've explained the vulnerable ERC721 token in my high-risk submission. (An order maker might steal premium from the sender by revoking exercise() using a vulnerable asset.)

1. Alice creates a long put order. premium = 0.1 WETH, strike = 1 WETH for 1000 USDC and 1 FAKE
2. Bob fills the order by depositing 1 WETH and receives 0.1 WETH.
3. Alice exercises the order by depositing 1000 USDC and 1 FAKE and receives 1 WETH.
4. After that, Bob tries to withdraw tokens but it fails to transfer FAKE.

So it wouldn't be profitable for Alice but Bob won't receive anything after paying 0.9 WETH (strike - premium).

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Same as the high-risk finding, it's difficult to suggest a perfect solution to fix this issue but I think it would be good to add some kind of whitelists for ERC20 and ERC721 that can be used while creating a new order.

Lines of code

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

Vulnerability details

Impact

Ensure zero msg.value if transferring from user and baseAsset is not WETH. A user that mistakenly calls either fillOrder() or exercise() with native ETH when baseAsset is not WETH, his native ETH will be locked in the contract.

Proof of Concept

There is a reference of the same issue.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should ensure msg.value == 0 for above cases.

} else {
    require(msg.value == 0, "ETH sent");
    ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
}

Lines of code

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

Vulnerability details

Impact

PuttyV2.withdraw() might revert with zero transfer. So users can't withdraw their funds and tokens.

Proof of Concept

As we can check from previous issue, some ERC20 tokens don't allow transfer of 0 amount. And when we calculate feeAmount, feeAmount might be zero even though the fee is positive when order.strike is less than 1000. Then the fee transfer might revert and users can't withdraw their funds and tokens.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We can modify this part like below.

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

Low Risk Issues

1. PuttyV2.fillOrder() might revert when order.premium = 0. I think it's possible premium = 0 and zero tranfer might be failed for some ERC20 tokens from this reference. We should check premium != 0 before transfer.

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

Non-critical Issues

1. Constants should be defined rather than using magic numbers

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

2. Event is missing indexed fields. Each event should use three indexed fields if there are three or more fields

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

1. Use != 0 instead of > 0 for uint variables

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L293
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L327
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L351
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L427
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L498
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L599

2. No need to initialize variables with default values

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L497
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L556
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L594
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L611
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L627
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L637
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L647
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L658
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L670
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L728
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L742

3. Use ++i instead of i++, i+=1, also unchecked increments in for-loops will save gas cost

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L556
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L594
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L611
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L627
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L637
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L647
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L658
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L670
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L728
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L742

4. An array’s length should be cached to save gas in for-loops

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L556
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L594
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L611
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L627
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L637
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L647
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L658
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L670
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L728
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L742

5. Non-strict inequalities are cheaper than strict ones We can change "<" to "<=" as it isn't different much.

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

6. public functions not called by the contract should be declared as external

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L389
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L466
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L546
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L573
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L753