Putty contest - Metatron's results

Lines of code

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

Vulnerability details

The solution is not supporting maker shorting a call of N floorToken (although platform declare it support all four types: short\long call\put)

Proof of Concept

Exercising an option is done one-sided by the long taker, at this point the floorToken(s) should be transferred to him, but maker does not have the chance to specify what token_id(s) he want to use as 'floor' (the cheapest). no code to collateralize the maker possition is present in the solution.

Discussing with developers over DM on discord - verifies that shorting a call with floorToken is currently impossible.

Recommended Mitigation Steps

Maker can be collateralize with any N items from the collection, and have a chance to later send the cheapest, in replace of the collateral. In this manner, the collateral is guaranteed to cover the N floorTokens - in the worst case the most cheap items are the collateral itself. If maker is not sending cheaper items in the agreed time period - the taker can just take the N collateralized items.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L451 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L443-L457

Vulnerability details

Owner\creators lose their share in the profit by not collecting the fees on half the exercised cases (all puts).

The only place where owner receives fee is when withdrawing an exercised call or expired put: https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506

But when exercising a put, current code is transferring the whole strike to the exerciser (long).

ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

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

Proof of Concept

Consider the following test based on testItWithdrawsAssetsIfPutExercised

    function testItWithdrawsAssetsIfPutExercisedWithFees() public {
        // arrange
        
        console.log("Setting fees to 2%");
        p.setFee(20);

        PuttyV2.Order memory order = defaultOrder();

        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));
        console.log("balance of bob: ", ERC20(order.baseAsset).balanceOf(bob));

        order.isCall = false;
        order.isLong = false;

        console.log("balance of owner at start: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        uint256 erc721TokenId = 88;
        bayc.mint(address(bob), erc721TokenId);
        vm.prank(bob);
        bayc.setApprovalForAll(address(p), true);
        erc721Assets.push(PuttyV2.ERC721Asset({token: address(bayc), tokenId: erc721TokenId}));
        order.erc721Assets = erc721Assets;

        bytes memory signature = signOrder(babePrivateKey, order);
        vm.prank(bob);
        link.approve(address(p), type(uint256).max);
        vm.prank(bob);
        p.fillOrder(order, signature, floorAssetTokenIds);

        console.log("balance of owner after fill: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        PuttyV2.Order memory longOrder = abi.decode(abi.encode(order), (PuttyV2.Order)); // decode/encode to get a copy instead of reference
        longOrder.isLong = true;

        vm.prank(bob);
        p.exercise(longOrder, floorAssetTokenIds);
        
        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));

        console.log("balance should grow by 2% of strike (403001) in compare to when fee is 0%");

        console.log("balance of owner after exercise: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        // act
        vm.prank(babe);
        p.withdraw(order);

        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));
        console.log("balance of bob: ", ERC20(order.baseAsset).balanceOf(bob));

        console.log("balance of owner after withdraw: ", ERC20(order.baseAsset).balanceOf(p.owner()));

    }

When using code as it is, setting the fees to any value including 0 result in the same output (In contrast to the fee being collected on Call options)

  balance of babe: , 4294967295
  balance of bob: , 4294967295
  balance of owner at start: , 4294967295
  balance of owner after fill: , 4294967295
  balance of babe: , 4274822143
  balance of owner after exercise: , 4294967295
  balance of babe: , 4274822143
  balance of bob: , 4315112447
  balance of owner after withdraw: , 4294967295

When running the test with 2% fee and my code suggested in mitigation, results show correct handling of the fee:

  Setting fees to 2%
  balance of babe: , 4294967295
  balance of bob: , 4294967295
  balance of owner at start: , 4294967295
  balance of owner after fill: , 4294967295
  balance of babe: , 4274822143
  balance should grow by 2% of strike (403001) in compare to when fee is 0%
  balance of owner after exercise: , 4295370296
  balance of babe: , 4274822143
  balance of bob: , 4314709446
  balance of owner after withdraw: , 4295370296

Tools

forge, editor, code review

Recommended Mitigation Steps

Instead of line L451 mentioned above, add this code:

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

Lines of code

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

Vulnerability details

Impact

Fee rates are not locked when fillOrder - might be unfair because admin can change contract fees unannounced. Assume an arbitrager is checking the current fees on the contract, decide it's worth and fillOrder on chain. Admin can change fees as he likes (in range 0-3%) and the new fees will be takin in account on exercise\withdraw. This unexpected and unfair behaviour might drive players and users out of the platform.

Proof of Concept

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

            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

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

Wherever fee is taken in account, the current fee rate fee is used instead of the fee at the time of the fillOrder.

Recommended Mitigation Steps

The easiest way to do so is to save the feeAmount or the fee_rate (fee): mapping(uint256 => uint256) public feeForOrder; on the contract level feeForOrder[orderHash] = feeAmount; on the fillOrder after the feeAmount is calculated.

then using this cached value then transferring fee to owner()

[L-01] Can cancel the same order multiple times.

Effects: In case it was already filled, this operation:

1. Emit a misleading CancelledOrder event,
2. Not informative to maker as his call to cancel was not reverted.

Mitigation:

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

    bytes32 orderHash = hashOrder(order);
			
    require(!cancelledOrders[orderHash], "Order already cancelled");  //@audit add this line here

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

    emit CancelledOrder(orderHash, order);
}

[N-01] Ambiguous error message in batchFillOrder:

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

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

Can specify which two mismatch to help user re-send correct transaction.

[N-02] Ambiguous error message in constructor:

require(_weth != address(0), "Unset weth address"); https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L214

Better to say "Invalid weth address" or "weth address cannot be 0"

[N-03] Inconsistent format of error messages

All error messages on PuttyV2Nft.sol are ALL_CAPICATAL and snake cased, while all error messages on PuttyV2.sol are natural English language (only first letter is capital and use spaces) "INVALID_RECIPIENT" in compare to "Length mismatch in input"

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

[G-00] Expensive if can be avoided

contracts/src/PuttyV2.sol:
  494              // transfer strike to owner if put is expired or call is exercised 
  495:             if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
  496                  // send the fee to the admin/DAO if fee is greater than 0%

  508              // transfer assets from putty to owner if put is exercised or call is expired
  509:             if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) {
  510                  _transferERC20sOut(order.erc20Assets);

Given that order.isCall and isExercised are boolean, there are only 4 outcomes to their Cartesian product. 2 cases are covered in the first if (L494) and so the other 2 cases can be in else of the first if, instead of computing again. Considering that first if have return; at the end, you can even skip the else. This will save gas of at least 4 reads and 1 logic op every time withdraw is called.

[G-01] ++i costs less gas compared to i++

++i costs about 5 gas less per iteration compared to i++ for unsigned integer. This statement is true even with the optimizer enabled. Summarized my results where i is used in a loop, is unsigned integer, and you safely can be changed to ++i without changing any behavior

I've found 11 locations:

contracts/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(

[G-02] An arrays length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset). Caching the array length in the stack saves around 3 gas per iteration. I've found 10 locations:

contracts/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(

Furthermore - it might be a waist to cache length if it's likely to be 0 - but if you think that this is the case, I would recommend: [G-02B] Check that the length of the first parameter is bigger than zero, before even calling the function, and save the function call.

593 function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {

610 function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {

622 function _transferFloorsIn(

636 function _transferERC20sOut(ERC20Asset[] memory assets) internal {

646 function _transferERC721sOut(ERC721Asset[] memory assets) internal {

657 function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {

[G-03] Using default values is cheaper than assignment

If a variable is not set/initialized, it is assumed to have the default value, 0 for uint Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example: uint8 i = 0; should be replaced with uint8 i;

I've found 11 locations:

contracts/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) {

  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(

[G-04] Upgrade pragma to 0.8.15 to save gas

Across the whole solution, the declared pragma is 0.8.13 According to the release note of 0.8.15: https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/ The benchmark shows saving of 2.5-10% Bytecode size, Saving 2-8% Deployment gas, And saving up to 6.2% Runtime gas.

[G-05] Custom errors save gas

From solidity 0.8.4 and above, Custom errors from are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

I've found 33 require locations in 2 different files:

contracts/src/PuttyV2.sol:
  213          ) {
  214:             require(_weth != address(0), "Unset weth address");
  215  

  240          function setFee(uint256 _fee) public payable onlyOwner {
  241:             require(_fee < 30, "fee must be less than 3%");
  242  

  277              // check signature is valid using EIP-712
  278:             require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");
  279  
  280              // check order is not cancelled
  281:             require(!cancelledOrders[orderHash], "Order has been cancelled");  
  282  
  283              // check msg.sender is allowed to fill the order
  284:             require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
  285  
  286              // check duration is valid
  287:             require(order.duration < 10_000 days, "Duration too long");
  288  
  289              // check order has not expired
  290:             require(block.timestamp < order.expiration, "Order has expired");
  291  
  292              // check base asset exists
  293:             require(order.baseAsset.code.length > 0, "baseAsset is not contract");
  294  

  296              order.isCall && order.isLong  
  297:                 ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
  298:                 : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");
  299  

  328                      // check enough ETH was sent to cover the premium
  329:                     require(msg.value == order.premium, "Incorrect ETH amount sent");
  330  

  352                      // check enough ETH was sent to cover the strike
  353:                     require(msg.value == order.strike, "Incorrect ETH amount sent");
  354  

  394              // check user owns the position
  395:             require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
  396  
  397              // check position is long
  398:             require(order.isLong, "Can only exercise long positions");
  399  
  400              // check position has not expired
  401:             require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
  402  

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

  428                      // check enough ETH was sent to cover the strike
  429:                     require(msg.value == order.strike, "Incorrect ETH amount sent");
  430  

  469              // check order is short
  470:             require(!order.isLong, "Must be short position");
  471  

  474              // check msg.sender owns the position
  475:             require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
  476  

  480              // check long position has either been exercised or is expired
  481:             require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
  482  

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

  550          ) public returns (uint256[] memory positionIds) {
  551:             require(orders.length == signatures.length, "Length mismatch in input");
  552:             require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");
  553  

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

  764          function tokenURI(uint256 id) public view override returns (string memory) {
  765:             require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");
  766  

contracts/src/PuttyV2Nft.sol:
  11      function _mint(address to, uint256 id) internal override {
  12:         require(to != address(0), "INVALID_RECIPIENT");
  13:         require(_ownerOf[id] == address(0), "ALREADY_MINTED");
  14  

  25      ) public override {
  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          );

  40      function balanceOf(address owner) public pure override returns (uint256) {
  41:         require(owner != address(0), "ZERO_ADDRESS");
  42          return type(uint256).max;

[G-06] != 0 is cheaper than > 0

!= 0 costs less gas compared to > 0 for unsigned integers even when optimizer enabled ** saves 6 gas **

I've found 7 locations:

contracts/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