Lack Of 0 Amount Check Allows Malicious User To Create Large Number Of Orders

[code423n4]

Lines of code

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

Vulnerability details

Proof-of-Concept

Based on the implementation of the PuttyV2.fillOrder, it was found that there is no validation to ensure that the order.premium and order.strike is not zero. Thus, a griefer can create as many orders as they want in Putty.

This will most likely pose problems on the front-end of the Putty protocol because there will be a ridiculously high number of "spam" orders displayed to actual users. This affects the usability of the protocol, and damage Putty's reputation. Malicious users could easily fill up the "Open Orders" and "Filled Orders" page of Putty Protocol. Malicious Users could easily fill up the "Open Order" and "Filled Order" pages in Putty.

Following are the only checks implemented in the PuttyV2.fillOrder. Note that it does not validate that the order.premium and order.strike is not zero on-chain.

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

function fillOrder(
	Order memory order,
	bytes calldata signature,
	uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {

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

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

Attacker could perform the following:

1. Create 2000 long call orders with order.strike and order.premium set to zero.
2. Submit 2000 orders off-chain to Putty
3. Attempt to fill 1000 orders by calling PuttyV2.fillOrder function.
4. End Result = 1000 open order will be displayed in the "Open Order" page, and 1000 filled order will be displayed in the "Filled Order" page on Putty front-end.

Impact

This affects the usability of the protocol, and damage Putty's reputation.

Recommended Mitigation Steps

It is recommended to add the following checks to ensure that the order.strike and order.premium is not equal to zero

function fillOrder(
	Order memory order,
	bytes calldata signature,
	uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {

	bytes32 orderHash = hashOrder(order);
	
	require(order.strike > 0 && order.premium > 0, "Amount must be greater than 0");
	..SNIP..
}

Although client-side or off-chain might have already implemented such validations, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented in the on-chain contracts.

Additionally, consider restricting the number of orders each user can create or introduce a time delay that user has to wait after creating a new order to reduce the impact of this issue.

[outdoteth]

Should be tagged as low severity imo.
Spam orders are prevented at the db level already.
The proposed onchain check does not solve the issue either because a user can still create orders with a premium of 1 wei or a strike of 1 wei which is almost the same as a 0 wei order.

Also note that options with a strike of 0 are valid orders with a use case.

[outdoteth]

Report: Possible to create spam orders with 0 strike and premium

[HickupHH3]

As #418 mentioned, 0 strike options are a common derivative. Agree with sponsor that checking > 0 isn't sufficient as a sanity check. Maybe have a minimum premium amount depending on token, though this adds overhead.

Spam / DoS attacks also require gas to execute. Using ETH mainnet acts as a natural deterrence, but are more viable on L2s etc.

The front-end can choose to filter out such orders of dust amounts as well.

Hence, am downgrading the issue to QA.

[HickupHH3]

Part of warden's QA report: #306