Putty contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L636-L640 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L646-L650 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L657-L661

Vulnerability details

Impact

There are no bounds on the number of tokens transferred in an order, and gas requirements can change (especially since orders can have a duration of 27 years), so orders filled at time T1 may not be exercisable/withdrawable at time T2, or with the provided assets if the assets use a lot of gas during their transfers (e.g. aTokens and cTokens). The buyer of the option will have paid the premium, and will be unable to get the assets they are owed.

Proof of Concept

There are no upper bounds on the number of assets being transferred in these loops:

File: contracts/src/PuttyV2.sol   #1

636       function _transferERC20sOut(ERC20Asset[] memory assets) internal {
637           for (uint256 i = 0; i < assets.length; i++) {
638               ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);
639           }
640       }

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

File: contracts/src/PuttyV2.sol   #2

646       function _transferERC721sOut(ERC721Asset[] memory assets) internal {
647           for (uint256 i = 0; i < assets.length; i++) {
648               ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);
649           }
650       }

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

File: contracts/src/PuttyV2.sol   #3

657       function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
658           for (uint256 i = 0; i < floorTokens.length; i++) {
659               ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);
660           }
661       }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Have an upper bound on the number of assets, or allow them to be transferred out one at a time, if necessary

Lines of code

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

Vulnerability details

Impact

Put option buyers pay an option premium to the seller for the privilege of being able to 'put' assets to the seller and get the strike price for it rather than the current market price. If they're unable to perform the 'put', they've paid the premium for nothing, and essentially have had funds stolen from them.

Proof of Concept

If the put option seller includes in order.erc20Assets, an amount of zero for any of the assets, or specifies an asset that doesn't currently have any code at its address, the put buyer will be unable to exercise the option, and will have paid the premium for nothing:

File: contracts/src/PuttyV2.sol   #1

453               // transfer assets from exerciser to putty
454               _transferERC20sIn(order.erc20Assets, msg.sender);

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

The function reverts if any amount is equal to zero, or the asset doesn't exist:

File: contracts/src/PuttyV2.sol   #2

593       function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
594           for (uint256 i = 0; i < assets.length; i++) {
595               address token = assets[i].token;
596               uint256 tokenAmount = assets[i].tokenAmount;
597   
598               require(token.code.length > 0, "ERC20: Token is not contract");
599               require(tokenAmount > 0, "ERC20: Amount too small");
600   
601               ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
602           }
603       }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Verify the asset amounts and addresses during fillOrder(), and allow exercise if the token no longer exists at that point in time

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

Vulnerability details

Impact

fillOrder() and exercise() have code paths that require Ether to be sent to them (e.g. using WETH as the base asset, or the provision of the exercise price), and therefore those two functions have the payable modifier. However, there are code paths within those functions that do not require Ether. Ether passed to the functions, when the non-Ether code paths are taken, is locked in the contract forever, and the sender gets nothing extra in return for it.

Proof of Concept

Ether can't be pulled from the order.maker during the filling of a long order, so msg.value shouldn't be provided here:

File: contracts/src/PuttyV2.sol   #1

323           if (order.isLong) {
324               ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
325           } else {

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

If the baseAsset isn't WETH during order fulfillment, msg.value is unused:

File: contracts/src/PuttyV2.sol   #2

337               } else {
338                   ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
339               }

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

Same for the exercise of call options:

File: contracts/src/PuttyV2.sol   #3

435               } else {
436                   ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
437               }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Add a require(0 == msg.value) for the above three conditions

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

Lines of code

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

Vulnerability details

Impact

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function.

Putty uses ERC-721 tokens as the method of tracking put/call ownership, and if they're sent to an address that cannot handle them, the options will be unusable and worthless.

Proof of Concept

Orders are filled by the buyer of the option, not the order.maker, so the maker may have specified an address that cannot handle NFTs without realizing it. They'll only realize their mistake when they find that they have no way to exercise an in-the-money position:

File: contracts/src/PuttyV2.sol   #1

303           _mint(order.maker, uint256(orderHash));

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

The taker too may be unable to handle NFTs, but this is less likely as they're the one interacting with the Putty contracts directly as msg.sender:

File: contracts/src/PuttyV2.sol   #2

305           // create opposite long/short position for taker
306           bytes32 oppositeOrderHash = hashOppositeOrder(order);
307           positionId = uint256(oppositeOrderHash);
308           _mint(msg.sender, positionId);

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

Tools Used

Code inspection

Recommended Mitigation Steps

Use _safeMint() instead of _mint()

Lines of code

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

Vulnerability details

Impact

A necessary part of a put option is the receipt of the assets 'put' to the put-option seller, when the put buyer exercises the option. If the seller is unable to get the assets put to them, they lose their capital.

Proof of Concept

If order.baseAsset is a token that reverts if the amount transferred is zero, and the fee ends up being zero, all attempts to withdraw assets will fail:

File: contracts/src/PuttyV2.sol   #1

499                   feeAmount = (order.strike * fee) / 1000;
500                   ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);

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

LEND is a specific example of an extant token that has this behavior, and will cause assets to be locked.

Tools Used

Code inspection

Recommended Mitigation Steps

Do not call safeTransfer() if feeAmount is zero

Summary

Low Risk Issues

Total: 7 instances over 5 issues

Non-critical Issues

Total: 31 instances over 12 issues

Low Risk Issues

1. Putty does not follow ERC721 standard

balanceOf() returns the wrong value for all users. It's not 'reasonable' to disregard the standard to just save gas. The purpose of a standard is to ensure interoperability, and this current code breaks that. We file a lot of medium-risk issues on various projects because Tether (USDT) does things in a non-standard way which leads to bugs in other people's code because they don't know about the idiosyncrasies. This project is introducing yet another potential attack vector.

There is 1 instance of this issue:

File: contracts/src/PuttyV2Nft.sol   #1

7    // removes balanceOf modifications
8    // questionable tradeoff but given our use-case it's reasonable
9:   abstract contract PuttyV2Nft is ERC721("Putty", "OPUT") {

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

2. NatSpec function description is misleading

Not all orders can be batched; orders using native Ether intentionally cannot be batched. This limitation should be outlined in the NatSpec

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

539      /**
540          @notice Batch fills multiple orders.
541          @param orders The orders to fill.
542          @param signatures The signatures to use for each respective order.
543          @param floorAssetTokenIds The floorAssetTokenIds to use for each respective order.
544          @return positionIds The ids of the position NFT that the msg.sender receives.
545       */
546      function batchFillOrder(
547          Order[] memory orders,
548          bytes[] calldata signatures,
549          uint256[][] memory floorAssetTokenIds
550:     ) public returns (uint256[] memory positionIds) {

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

3. Fees can be bypassed using wrapper tokens

If a user wishes to avoid fees, they can wrap their base asset in another token with fewer decimals (e.g. none) and cause the fee to be zero. Obviously this cannot be solved for custom tokens that have arbitrary logic, but tokens that deterministically scale amounts (e.g. one where balanceOf() returns underlying.balanceOf() / 1e18 can be dealt with by rejecting any order where the fee ends up being zero during fillOrder()

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

499                  feeAmount = (order.strike * fee) / 1000;
500:                 ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);

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

4. No support for Punks, Kitties, and ERC-1155s

CryptoPunks and CryptoKitties came before the ERC-721 standard and thus do not support them. A lot of new NFTs follow the ERC-1155 standard rather than the ERC-721 standard. Consider adding Putty-specific wrappers for these sorts of NFTs so that your users don't have to look elsewhere to sell their NFTs

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

610      function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
611          for (uint256 i = 0; i < assets.length; i++) {
612              ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);
613          }
614:     }

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

5. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

If all arguments are strings and or bytes, bytes.concat() should be used instead

There are 3 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

90:           keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

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

File: contracts/src/PuttyV2.sol   #2

96:           keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

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

File: contracts/src/PuttyV2.sol   #3

102           keccak256(
103               abi.encodePacked(
104                   "Order(",
105                   "address maker,",
106                   "bool isCall,",
107                   "bool isLong,",
108                   "address baseAsset,",
109                   "uint256 strike,",
110                   "uint256 premium,",
111                   "uint256 duration,",
112                   "uint256 expiration,",
113                   "uint256 nonce,"
114                   "address[] whitelist,",
115                   "address[] floorTokens,",
116                   "ERC20Asset[] erc20Assets,",
117                   "ERC721Asset[] erc721Assets",
118                   ")",
119                   "ERC20Asset(address token,uint256 tokenAmount)",
120                   "ERC721Asset(address token,uint256 tokenId)"
121               )
122:          );

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

Non-critical Issues

1. Incomplete description of functionality

Not only is EIP-712 checked, but isValidSignatureNow() also does EIP-1271 contract verification as well

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

277          // check signature is valid using EIP-712
278:         require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

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

2. Consider revert()ing if final if-condition is not satisfied

The fillOrder() function has a series of if-statements covering all currently-possible scenarios. If, however, in the future new scenarios are added, one of the conditions may be missed, and the function will return the uninitialized return variable. Consider adding a revert(false) to the end of the function to catch any such mistakes

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

268      function fillOrder(
269          Order memory order,
270          bytes calldata signature,
271          uint256[] memory floorAssetTokenIds
272:     ) public payable returns (uint256 positionId) {

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

3. Avoid the use of sensitive terms

Use alternative variants, e.g. allowlist/denylist instead of whitelist/blacklist

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

284:         require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

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

4. Don't use periods with fragments

Throughout the files, most of the comments have fragments that end with periods. They should either be converted to actual sentences with both a noun phrase and a verb phrase, or the periods should be removed.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol (various lines)   #1

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

5. Adding a return statement when the function defines a named return variable, is redundant

There are 4 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

345:              return positionId;

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

File: contracts/src/PuttyV2.sol   #2

363:              return positionId;

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

File: contracts/src/PuttyV2.sol   #3

370:              return positionId;

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

File: contracts/src/PuttyV2.sol   #4

378:              return positionId;

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

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

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 5 instances of this issue:

File: contracts/src/PuttyV2.sol

389:      function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {

466:      function withdraw(Order memory order) public {

546       function batchFillOrder(
547           Order[] memory orders,
548           bytes[] calldata signatures,
549           uint256[][] memory floorAssetTokenIds
550:      ) public returns (uint256[] memory positionIds) {

573       function acceptCounterOffer(
574           Order memory order,
575           bytes calldata signature,
576           Order memory originalOrder
577:      ) public payable returns (uint256 positionId) {

753:      function domainSeparatorV4() public view returns (bytes32) {

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

7. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 5 instances of this issue:

File: contracts/src/PuttyV2.sol

/// @audit 30
241:          require(_fee < 30, "fee must be less than 3%");

/// @audit 10_000
287:          require(order.duration < 10_000 days, "Duration too long");

/// @audit 0xdead
413:          transferFrom(msg.sender, address(0xdead), uint256(orderHash));

/// @audit 0xdead
488:          transferFrom(msg.sender, address(0xdead), uint256(orderHash));

/// @audit 1000
499:                  feeAmount = (order.strike * fee) / 1000;

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

8. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

There are 3 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

89        bytes32 public constant ERC721ASSET_TYPE_HASH =
90:           keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

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

File: contracts/src/PuttyV2.sol   #2

95        bytes32 public constant ERC20ASSET_TYPE_HASH =
96:           keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

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

File: contracts/src/PuttyV2.sol   #3

101       bytes32 public constant ORDER_TYPE_HASH =
102           keccak256(
103               abi.encodePacked(
104                   "Order(",
105                   "address maker,",
106                   "bool isCall,",
107                   "bool isLong,",
108                   "address baseAsset,",
109                   "uint256 strike,",
110                   "uint256 premium,",
111                   "uint256 duration,",
112                   "uint256 expiration,",
113                   "uint256 nonce,"
114                   "address[] whitelist,",
115                   "address[] floorTokens,",
116                   "ERC20Asset[] erc20Assets,",
117                   "ERC721Asset[] erc721Assets",
118                   ")",
119                   "ERC20Asset(address token,uint256 tokenAmount)",
120                   "ERC721Asset(address token,uint256 tokenId)"
121               )
122:          );

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

9. Typos

There are 3 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

/// @audit offchain
260:          @notice Fills an offchain order and settles it onchain. Mints two

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

File: contracts/src/PuttyV2.sol   #2

/// @audit onchain
260:          @notice Fills an offchain order and settles it onchain. Mints two

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

File: contracts/src/PuttyV2.sol   #3

/// @audit seperator
751:          @return The domain seperator used when calculating the eip-712 hash.

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

10. File is missing NatSpec

There is 1 instance of this issue:

File: contracts/src/PuttyV2Nft.sol (various lines)   #1

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

11. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There are 2 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

171:      event NewBaseURI(string baseURI);

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

File: contracts/src/PuttyV2.sol   #2

177:      event NewFee(uint256 fee);

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

12. Duplicated require()/revert() checks should be refactored to a modifier or function

The compiler will inline the function, which will avoid JUMP instructions usually associated with functions

There are 4 instances of this issue:

File: contracts/src/PuttyV2Nft.sol   #1

27:           require(to != address(0), "INVALID_RECIPIENT");

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

File: contracts/src/PuttyV2.sol   #2

405:              ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")

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

File: contracts/src/PuttyV2.sol   #3

429:                  require(msg.value == order.strike, "Incorrect ETH amount sent");

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

File: contracts/src/PuttyV2.sol   #4

475:          require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

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

Summary

Gas Optimizations

Total: 98 instances over 15 issues

Gas Optimizations

1. Using EIP-712 hashes for positionIds wastes gas

EIP-712 introduces a lot of metadata into the data being hashed, in order for it to be able to be read before the data is signed. When the signature isn't being checked, it's inefficient to use these hashes over normal keccak256() hashes. This gist shows that the most simple order (all zero-byte values, no internal arrays) has an overhead of 658 gas as compared to normal hashing. More complicated orders, especially with internal arrays, will save even more. Message signing should be separate from positionId calculation, which will save gas. These hashes are used in all of the major functions of the project, so savings over time will be significant

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

305          // create opposite long/short position for taker
306          bytes32 oppositeOrderHash = hashOppositeOrder(order);
307          positionId = uint256(oppositeOrderHash);
308:         _mint(msg.sender, positionId);

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

2. Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

149       mapping(uint256 => uint256) public positionExpirations;
150   
151       /**
152           @notice Whether or not a position has been exercised. Maps 
153                   from positionId to isExercised.
154       */
155       mapping(uint256 => bool) public exercisedPositions;
156   
157       /**
158           @notice The floor asset token ids for a position. Maps from 
159                   positionId to floor asset token ids. This should only 
160                   be set for a long call position in `fillOrder`, or for 
161                   a short put position in `exercise`.
162       */
163:      mapping(uint256 => uint256[]) public positionFloorAssetTokenIds;

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

3. Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 7 instances of this issue:

File: contracts/src/PuttyV2.sol

/// @audit _baseURI
209       constructor(
210           string memory _baseURI,
211           uint256 _fee,
212:          address _weth

/// @audit order
389:      function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {

/// @audit order
466:      function withdraw(Order memory order) public {

/// @audit orders
546       function batchFillOrder(
547           Order[] memory orders,
548           bytes[] calldata signatures,
549           uint256[][] memory floorAssetTokenIds
550:      ) public returns (uint256[] memory positionIds) {

/// @audit floorAssetTokenIds
546       function batchFillOrder(
547           Order[] memory orders,
548           bytes[] calldata signatures,
549           uint256[][] memory floorAssetTokenIds
550:      ) public returns (uint256[] memory positionIds) {

/// @audit order
573       function acceptCounterOffer(
574           Order memory order,
575           bytes calldata signature,
576           Order memory originalOrder
577:      ) public payable returns (uint256 positionId) {

/// @audit originalOrder
573       function acceptCounterOffer(
574           Order memory order,
575           bytes calldata signature,
576           Order memory originalOrder
577:      ) public payable returns (uint256 positionId) {

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

4. State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

/// @audit fee on line 498
499:                  feeAmount = (order.strike * fee) / 1000;

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

5. Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata

There are 6 instances of this issue:

File: contracts/src/PuttyV2.sol

/// @audit assets[i] on line 595
596:              uint256 tokenAmount = assets[i].tokenAmount;

/// @audit assets[i] on line 612
612:              ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);

/// @audit assets[i] on line 638
638:              ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);

/// @audit assets[i] on line 648
648:              ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);

/// @audit arr[i] on line 731
731:                  keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))

/// @audit arr[i] on line 745
745:                  keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))

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

6. <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 10 instances of this issue:

File: contracts/src/PuttyV2.sol

556:          for (uint256 i = 0; i < orders.length; i++) {

594:          for (uint256 i = 0; i < assets.length; i++) {

611:          for (uint256 i = 0; i < assets.length; i++) {

627:          for (uint256 i = 0; i < floorTokens.length; i++) {

637:          for (uint256 i = 0; i < assets.length; i++) {

647:          for (uint256 i = 0; i < assets.length; i++) {

658:          for (uint256 i = 0; i < floorTokens.length; i++) {

670:          for (uint256 i = 0; i < whitelist.length; i++) {

728:          for (uint256 i = 0; i < arr.length; i++) {

742:          for (uint256 i = 0; i < arr.length; i++) {

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

7. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 10 instances of this issue:

File: contracts/src/PuttyV2.sol

556:          for (uint256 i = 0; i < orders.length; i++) {

594:          for (uint256 i = 0; i < assets.length; i++) {

611:          for (uint256 i = 0; i < assets.length; i++) {

627:          for (uint256 i = 0; i < floorTokens.length; i++) {

637:          for (uint256 i = 0; i < assets.length; i++) {

647:          for (uint256 i = 0; i < assets.length; i++) {

658:          for (uint256 i = 0; i < floorTokens.length; i++) {

670:          for (uint256 i = 0; i < whitelist.length; i++) {

728:          for (uint256 i = 0; i < arr.length; i++) {

742:          for (uint256 i = 0; i < arr.length; i++) {

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

8. Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

/// @audit setBaseURI(), setFee(), fillOrder(), exercise(), withdraw(), cancel(), batchFillOrder(), acceptCounterOffer(), isWhitelisted(), hashOppositeOrder(), hashOrder(), encodeERC20Assets(), encodeERC721Assets(), domainSeparatorV4()
53:   contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Ownable {

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

9. Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 2 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

143:      mapping(bytes32 => bool) public cancelledOrders;

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

File: contracts/src/PuttyV2.sol   #2

155:      mapping(uint256 => bool) public exercisedPositions;

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

10. Using > 0 costs more gas than != 0 when used on a uint in a require() statement

This change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

599:              require(tokenAmount > 0, "ERC20: Amount too small");

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

11. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 11 instances of this issue:

File: contracts/src/PuttyV2.sol

497:              uint256 feeAmount = 0;

556:          for (uint256 i = 0; i < orders.length; i++) {

594:          for (uint256 i = 0; i < assets.length; i++) {

611:          for (uint256 i = 0; i < assets.length; i++) {

627:          for (uint256 i = 0; i < floorTokens.length; i++) {

637:          for (uint256 i = 0; i < assets.length; i++) {

647:          for (uint256 i = 0; i < assets.length; i++) {

658:          for (uint256 i = 0; i < floorTokens.length; i++) {

670:          for (uint256 i = 0; i < whitelist.length; i++) {

728:          for (uint256 i = 0; i < arr.length; i++) {

742:          for (uint256 i = 0; i < arr.length; i++) {

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

12. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 10 instances of this issue:

File: contracts/src/PuttyV2.sol

556:          for (uint256 i = 0; i < orders.length; i++) {

594:          for (uint256 i = 0; i < assets.length; i++) {

611:          for (uint256 i = 0; i < assets.length; i++) {

627:          for (uint256 i = 0; i < floorTokens.length; i++) {

637:          for (uint256 i = 0; i < assets.length; i++) {

647:          for (uint256 i = 0; i < assets.length; i++) {

658:          for (uint256 i = 0; i < floorTokens.length; i++) {

670:          for (uint256 i = 0; i < whitelist.length; i++) {

728:          for (uint256 i = 0; i < arr.length; i++) {

742:          for (uint256 i = 0; i < arr.length; i++) {

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

13. Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

89        bytes32 public constant ERC721ASSET_TYPE_HASH =
90:           keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

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

File: contracts/src/PuttyV2.sol   #2

95        bytes32 public constant ERC20ASSET_TYPE_HASH =
96:           keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

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

File: contracts/src/PuttyV2.sol   #3

101       bytes32 public constant ORDER_TYPE_HASH =
102           keccak256(
103               abi.encodePacked(
104                   "Order(",
105                   "address maker,",
106                   "bool isCall,",
107                   "bool isLong,",
108                   "address baseAsset,",
109                   "uint256 strike,",
110                   "uint256 premium,",
111                   "uint256 duration,",
112                   "uint256 expiration,",
113                   "uint256 nonce,"
114                   "address[] whitelist,",
115                   "address[] floorTokens,",
116                   "ERC20Asset[] erc20Assets,",
117                   "ERC721Asset[] erc721Assets",
118                   ")",
119                   "ERC20Asset(address token,uint256 tokenAmount)",
120                   "ERC721Asset(address token,uint256 tokenId)"
121               )
122:          );

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

14. Inverting the condition of an if-else-statement wastes gas

Flipping the true and false blocks instead saves 3 gas

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

404           !order.isCall
405               ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
406:              : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

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

15. Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 33 instances of this issue:

File: contracts/src/PuttyV2Nft.sol

12:           require(to != address(0), "INVALID_RECIPIENT");

13:           require(_ownerOf[id] == address(0), "ALREADY_MINTED");

26:           require(from == _ownerOf[id], "WRONG_FROM");

27:           require(to != address(0), "INVALID_RECIPIENT");

28            require(
29                msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
30                "NOT_AUTHORIZED"
31:           );

41:           require(owner != address(0), "ZERO_ADDRESS");

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

File: contracts/src/PuttyV2.sol

214:          require(_weth != address(0), "Unset weth address");

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

278:          require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

281:          require(!cancelledOrders[orderHash], "Order has been cancelled");

284:          require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

287:          require(order.duration < 10_000 days, "Duration too long");

290:          require(block.timestamp < order.expiration, "Order has expired");

293:          require(order.baseAsset.code.length > 0, "baseAsset is not contract");

297:              ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")

298:              : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

329:                  require(msg.value == order.premium, "Incorrect ETH amount sent");

353:                  require(msg.value == order.strike, "Incorrect ETH amount sent");

395:          require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

398:          require(order.isLong, "Can only exercise long positions");

401:          require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");

405:              ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")

406:              : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

429:                  require(msg.value == order.strike, "Incorrect ETH amount sent");

470:          require(!order.isLong, "Must be short position");

475:          require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

481:          require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

527:          require(msg.sender == order.maker, "Not your order");

551:          require(orders.length == signatures.length, "Length mismatch in input");

552:          require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");

598:              require(token.code.length > 0, "ERC20: Token is not contract");

599:              require(tokenAmount > 0, "ERC20: Amount too small");

765:          require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");

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