Putty contest - Limbooo's results

[low-1] Lack of accuracy upon small amount on fee calculation.

In solidity division can be zero; 9 / 10 = 0. However, all values in (order.strike * fee) / 1000 where (order.strike * fee) < 1000 will be unsettled for fee payment and would just transfer a zero value to the owner of the contract. (999 * 1) / 1000 = 0 where 1 is the fee of 0.01%. Imagine that one day the ether has gone above $1,000,000 or the ERC20(order.baseAsset) used has less decimals by nature with high value, the owner of the contract would not be able to get any profit which the fee calculation required to do.

PoC

In PuttyV2.sol#withdraw()#L499 the division happened and transfering the fee amount immediately:

src/PuttyV2.sol:
497 uint256 feeAmount = 0;
498 if (fee > 0) {
499: feeAmount = (order.strike * fee) / 1000;
500 ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
501 }

Recommendation

Use FixedPoint Arithmetic library or scale up amount before division to a minimum upon divider value, and scale down after the division. At least, if this not in your concern; skip the transfer if feeAmount == 0.

[non-2] Ignoring function return may affect user funds

When using transfer with WETH we need to assert the call so we make sure the transfer process completed successfully.

PoC

• PuttyV2.sol#fillOrder()#L336

src/PuttyV2.sol:
335 IWETH(weth).deposit{value: msg.value}();
336: IWETH(weth).transfer(order.maker, msg.value);

Recommendation

Use assertion to revert on failure

assert(IWETH(weth).transfer(order.maker, msg.value));

List of contents:

• Data location of Order structure in function argument could be calldata.
• > 0 is less efficient than != 0 for unsigned integers.
• Setting uint256 variables to 0 is redundant.
• Optimizing for-loop.
• External function consume less gas than public.

[1] Data location of Order structure in function argument could be calldata

From the Solidity docs: Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

Function affected:

• PuttyV2.sol#fillOrder()
• PuttyV2.sol#exercise()
• PuttyV2.sol#withdraw()
• PuttyV2.sol#cancel()
• PuttyV2.sol#batchFillOrder() used as array.
• PuttyV2.sol#acceptCounterOffer() originalOrder could be calldata too.
• PuttyV2.sol#hashOppositeOrder()
• PuttyV2.sol#hashOrder()

The manipulation of the order structure happen only once when calculates PuttyV2.sol#hashOppositeOrder() and this can be solved using this keyword allows us to pass memory params to a function that accepts calldata params. Gas diff table:

╭──────────────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮
│ src/PuttyV2.sol:PuttyV2 contract ┆                 ┆        ┆        ┆        ┆         │
╞══════════════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡
│ Deployment Cost                  ┆ Deployment Size ┆        ┆      ┆        ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 4774098                          ┆ 25226           ┆        ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                    ┆ min             ┆ avg    ┆ median ┆ max    ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ cancel                           ┆ 3075            ┆ 26509  ┆ 34321  ┆ 34321  ┆ 4       │
│ cancel                           ┆ 703             ┆ 26005  ┆ 34440  ┆ 34440  ┆ 4       │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ exercise                         ┆ 5759            ┆ 55920  ┆ 68002 ┆ 133534 ┆ 18      │
│ exercise                         ┆ 4731            ┆ 57637  ┆ 69551 ┆ 140619 ┆ 18      │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ fillOrder                        ┆ 10014           ┆ 106845 ┆ 115883 ┆ 203777 ┆ 51      │
│ fillOrder                        ┆ 9121            ┆ 112948 ┆ 123603 ┆ 212253 ┆ 51      │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ hashOrder                        ┆ 5206            ┆ 5484   ┆ 5206   ┆ 7782   ┆ 62      │
│ hashOrder                        ┆ 4178            ┆ 4592   ┆ 4178   ┆ 7011   ┆ 114     │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withdraw                         ┆ 3075            ┆ 27840  ┆ 24545 ┆ 71168  ┆ 10      │
│ withdraw                         ┆ 697             ┆ 31196  ┆ 28491 ┆ 78172  ┆ 10      │
╰──────────────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

[2] > 0 is less efficient than != 0 for unsigned integers

If you enable the optimizer at 10k AND you're in a require statement, this will save gas. I suggest changing > 0 with != 0 here:

src/PuttyV2.sol:
292          // check base asset exists
293:         require(order.baseAsset.code.length > 0, "baseAsset is not contract");
294  

326              // handle the case where the user uses native ETH instead of WETH to pay the premium
327:             if (weth == order.baseAsset && msg.value > 0) {
328                  // check enough ETH was sent to cover the premium

350              // handle the case where the taker uses native ETH instead of WETH to deposit the strike
351:             if (weth == order.baseAsset && msg.value > 0) {
352                  // check enough ETH was sent to cover the strike

426              // handle the case where the taker uses native ETH instead of WETH to pay the strike
427:             if (weth == order.baseAsset && msg.value > 0) {
428                  // check enough ETH was sent to cover the strike

497              uint256 feeAmount = 0;
498:             if (fee > 0) {
499                  feeAmount = (order.strike * fee) / 1000;

597  
598:             require(token.code.length > 0, "ERC20: Token is not contract");
599:             require(tokenAmount > 0, "ERC20: Amount too small");
600 

[3] Setting uint256 variables to 0 is redundant

Default value is zero no need to assign 0

src/PuttyV2.sol:
496              // send the fee to the admin/DAO if fee is greater than 0%
497:             uint256 feeAmount = 0;
498              if (fee > 0) {

[4] Optimizing for-loop

1. Using ++i consumes 5 less gas than i++
2. Unchecked{ ++i; } consumes 49 less gas each iteration
3. Default value is zero no need to assign i = 0

src/PuttyV2.sol:
555          
556:         for (uint256 i = 0; i < orders.length; i++) {
557              positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]);

593      function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
594:         for (uint256 i = 0; i < assets.length; i++) {
595              address token = assets[i].token;

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

626      ) internal {
627:         for (uint256 i = 0; i < floorTokens.length; i++) {
628              ERC721(floorTokens[i]).safeTransferFrom(from, address(this), floorTokenIds[i]);

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

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

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

669      function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) {
670:         for (uint256 i = 0; i < whitelist.length; i++) {
671              if (target == whitelist[i]) return true;

727      function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
728:         for (uint256 i = 0; i < arr.length; i++) {
729              encoded = abi.encodePacked(

741      function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {
742:         for (uint256 i = 0; i < arr.length; i++) {
743              encoded = abi.encodePacked(

[5] External function consume less gas than public

As for best practices, you should use external if you expect that the function will only ever be called externally, and use public if you need to call the function internally. Functions not used internally:

• PuttyV2.sol#exercise().
• PuttyV2.sol#withdraw().
• PuttyV2.sol#batchFillOrder().
• PuttyV2.sol#acceptCounterOffer().
• PuttyV2.sol#domainSeparatorV4().
• PuttyV2.sol#tokenURI().