Putty contest - 0xNineDec's results

Lines of code

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#L349 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L454

Vulnerability details

Impact

The PuttyV2._transferERC20sIn() function loops over every token that's part of an order. The amount of different ERC20 tokens that can conform an order is uncapped. If the array is big enough, the gas consumption of _transferERC20sIn() may exceed the current max. block size and the whole execution will be reverted. For put orders, this will prevent orders from being exercised causing a denial of service that prevents a user from taking profits of the held position.

Proof of Concept

Scenario A:

1. Alice creates and signs a long put offchain order with a considerable amount of different ERC20 tokens with a strike of 124 WETH, premium of 0.8 WETH for 2 BAYC.
2. Bob fills that order and because it is a long put the fillOrder call handles the premium transfer with line 324 and the collateral transfer with line 349. The order fulfillment of Bob does not call internally at any time _transferERC20sIn, _transferERC721sIn or _transferFloorsIn.

• 17 days pass, the floor price goes down and Alice wants to exercise her option. The contract tries to call _transferERC20sIn on line 454 and the max. block size is exceeded reverting the whole call.

As a result of this scenario, Alice will never be able to exercise her option. Bob will be forced to pay feeAmount while calling withdraw.

Scenario B:

1. Alice creates and signs a short put offchain order with a considerable amount of different ERC20 tokens with a strike of 124 WETH.
2. Bob fills the order which mints to himself a long side option and sends strike 124 WETH from Alice to PuttyV2.
3. 17 days pass and the floor goes considerably up and Bob decides to exercise his long option. The loop of _transferERC20sIn on line 454 will exceed the max. block size and Bob will never be able to take profits over his position.

This scenario is far more noxious than the first one because Alice is preventing Bob from taking profits of his position.

Recommended Mitigation Steps

Although max. block sizes can change in the future, in order to prevent this denial of service limiting the maximum amount of ERC20Assets can patch this. To do so, a max. amount cap can be checked with a require statement while filling an order. This will prevent orders with a high amount of elements on the ERC20Asset to be fulfilled and thus prevent the mentioned high gas consumption.

Lines of code

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

Vulnerability details

Impact

Currently the implementation of PuttyV2.setFee() can be changed at any time although it emits the respective event and has a maximum value. Because the owner() of this contract can be any address (multisig, EOA, DAO multisig, etc.) there are no insights on how are going to be changed the fees and when.

Proof of Concept

Sticking up to the contract code itself, the fee can be changed freely within the given range. This means that for instance a malicious owner can be monitoring the mempool, frontrun other benign users and increase the fee up to 3% whenever he wants which will convey in a sort of fraud. If the attack is wanted to be performed in an even more intricate way, the owner can also backrun the transaction and take the fee back to what it was before this process.

Recommended Mitigation Steps

In order to prevent this, timelocking the setFee function will help provide trust and predictability on how the protocol will work.

Q/A - N: KEY STRUCTS LACKING COMMENTS

Under PuttyV2.sol, although the ERC20Asset, ERC721Asset, Order structs inner variables are self-explanatory, it would increase even more the understanding of the protocol having comments explaining what means each variable and why it's important.

FOR LOOPS: ARRAY LENGTH CAN BE CACHED TO SAVE GAS

The following functions of PuttyV2.sol rely on for loops that read array.length at each iteration:

• _transferERC20sIn

• _transferERC721sIn

• _transferFloorsIn

• _transferERC20sOut

• _transferERC721sOut

• _transferFloorsOut

• isWhitelisted

• encodeERC20Assets

• encodeERC721Assets

Caching the array.length in the stack instead of checking i < arr.length saves around 3 gas units per iteration.

FOR LOOPS: USING ++i INSTEAD OF i++

The following functions update the counter with i++ which performs two memory steps (one to increment and other one to read the return value of the index) which can be modified with ++i (performing the update and return in one step) saving gas on each iteration.

• _transferERC20sIn

• _transferERC721sIn

• _transferFloorsIn

• _transferERC20sOut

• _transferERC721sOut

• _transferFloorsOut

• isWhitelisted

• encodeERC20Assets

• encodeERC721Assets

LOGIC COMPARISON: USING != 0 IS CHEAPER THAN > 0

While enabling the optimizer at 10k runs and being inside a require statement, this comparison strategy will save gas.

The following lines check that a value is greater than zero for unsigned integers.

• Line 293
• Line 598
• Line 599

CUSTOM ERRORS: USING CUSTOM ERRORS INSTEAD OF RETURN STRINGS

Custom errors from Solidity v0.8.13 provide a cheaper and cleaner way of feedback than revert strings.

Quoiting the Solidity docs

Errors in Solidity provide a convenient and gas-efficient way to explain to the user why an operation failed. They can be defined inside and outside of contracts (including interfaces and libraries). They have to be used together with the revert statement which causes all changes in the current call to be reverted and passes the error data back to the caller.

An example on how a custom error can be generated and implemented is within the official doc link from before, and they can be inherited.