Putty contest - BowTiedWardens's results

Lines of code

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

Vulnerability details

Issue: exercise is a payable function, yet msg.value is only handled in certain paths within the function.

Consequence: Sending ETH via msg.value in any of the following contexts will result in user funds being locked in the contract and lost.

• put option with any baseAsset
• call option not denominated in WETH

Mitigation: Add checks in each of these call paths requiring msg.value to be 0.

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Wrong price on chains other than Ethereum mainnet
  • 1.2. Funds can be locked
  • 1.3. _safeMint() should be used rather than _mint() wherever possible
  • 1.4. A malicious owner can keep the fee rate at zero, but if a large value transfer enters the mempool, the owner can jack the rate up to the maximum
  • 1.5. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 1.6. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. Fee On Transfer Token usage as order.baseAsset in fillOrder() will break exercise()
  • 2.2. Misleading comment
  • 2.3. Typos
  • 2.4. Use string.concat() or bytes.concat()
  • 2.5. It's better to emit after all processing is done
  • 2.6. public functions not called by the contract should be declared external instead

1. Low Risk Issues

1.1. Wrong price on chains other than Ethereum mainnet

PuttyV1 was deployed on both Ethereum and Optimism (native token: OP token). Here, it assumes that msg.value will be in Ether:

src/PuttyV2.sol:
  367:             if (weth == order.baseAsset && msg.value > 0) { 
  368                  // check enough ETH was sent to cover the premium
  369                  require(
  370:                     msg.value == order.premium,
  371                      "Incorrect ETH amount sent"

  377                  // 2) attack surface for re-entrancy is reduced
  378:                 IWETH(weth).deposit{value: msg.value}();
  379:                 IWETH(weth).transfer(order.maker, msg.value);

  401              // handle the case where the taker uses native ETH instead of WETH to deposit the strike
  402:             if (weth == order.baseAsset && msg.value > 0) {
  403                  // check enough ETH was sent to cover the strike
  404:                 require(msg.value == order.strike, "Incorrect ETH amount sent"); 
  405  

  497              // handle the case where the taker uses native ETH instead of WETH to pay the strike
  498:             if (weth == order.baseAsset && msg.value > 0) {
  499                  // check enough ETH was sent to cover the strike
  500:                 require(msg.value == order.strike, "Incorrect ETH amount sent");
  501  

  504                  // - because withdraw() assumes an ERC20 interface on the base asset.
  505:                 IWETH(weth).deposit{value: msg.value}();

This might turn wrong if not careful. Consider checking chainId before making this shortcut.

1.2. Funds can be locked

There isn't a withdraw mechanism and several payable methods are implemented:

PuttyV2.sol:245:    function setBaseURI(string memory _baseURI) public payable onlyOwner { 
PuttyV2.sol:257:    function setFee(uint256 _fee) public payable onlyOwner {
PuttyV2.sol:289:    ) public payable returns (uint256 positionId) {
PuttyV2.sol:450:        payable
PuttyV2.sol:678:    ) public payable returns (uint256 positionId) {

1.3. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

PuttyV2.sol:44:import "solmate/tokens/ERC721.sol";
PuttyV2.sol:337:        _mint(order.maker, uint256(orderHash));
PuttyV2.sol:342:        _mint(msg.sender, positionId);  

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful. The CEIP is already well respected in the solution.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.4. A malicious owner can keep the fee rate at zero, but if a large value transfer enters the mempool, the owner can jack the rate up to the maximum

The PuttyV2.withdraw() function fetches the current fee from the contract's state. This fee can be changed by the Admin/DAO.

File: PuttyV2.sol
257:     function setFee(uint256 _fee) public payable onlyOwner {
258:         require(_fee < 30, "fee must be less than 3%");
259: 
260:         fee = _fee;
261: 
262:         emit NewFee(_fee);
263:     }
264: 

Mitigation: Make sure a governance timelock is in place.

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

Similar issue in the past: here

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). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

PuttyV2.sol:112:            abi.encodePacked(
PuttyV2.sol:851:                keccak256(abi.encodePacked(order.whitelist)),
PuttyV2.sol:852:                keccak256(abi.encodePacked(order.floorTokens)),
PuttyV2.sol:872:            encoded = abi.encodePacked(
PuttyV2.sol:896:            encoded = abi.encodePacked( 

1.6. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

PuttyV2.sol:42:import "openzeppelin/access/Ownable.sol";
PuttyV2.sol:57:    Ownable
PuttyV2.sol:245:    function setBaseURI(string memory _baseURI) public payable onlyOwner { 
PuttyV2.sol:257:    function setFee(uint256 _fee) public payable onlyOwner {

2. Non-Critical Issues

2.1. Fee On Transfer Token usage as order.baseAsset in fillOrder() will break exercise()

As the protocol explicitly mentioned it wouldn't support Fee On Transfer or Rebase tokens, this is just an informative finding. Consider documentation to users that the protocol won't work with such tokens, so they don't fill orders with them.

2.2. Misleading comment

Issue with comment

As an auditor, I personally misunderstood this comment. Consider changing it:

File: PuttyV2.sol
- 273:             * It is also possible to cancel() an order before fillOrder() 
+ 273:             * It is only possible to cancel() an order before fillOrder() 

2.3. Typos

- PuttyV2.sol:910:        @return The domain seperator used when calculating the eip-712 hash. 
+ PuttyV2.sol:910:        @return The domain separator used when calculating the eip-712 hash. 

2.4. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

PuttyV2.sol:2:pragma solidity 0.8.13;
PuttyV2.sol:96:            abi.encodePacked("ERC721Asset(address token,uint256 tokenId)")
PuttyV2.sol:104:            abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)")
PuttyV2.sol:112:            abi.encodePacked(
PuttyV2.sol:851:                keccak256(abi.encodePacked(order.whitelist)),
PuttyV2.sol:852:                keccak256(abi.encodePacked(order.floorTokens)),
PuttyV2.sol:872:            encoded = abi.encodePacked(
PuttyV2.sol:896:            encoded = abi.encodePacked( 

2.5. It's better to emit after all processing is done

• src/PuttyV2.sol:

  354:         emit FilledOrder(orderHash, floorAssetTokenIds, order);
  355  
  356          /* ~~~ INTERACTIONS ~~~ */
  357  
  358          // transfer premium to whoever is short from whomever is long
  359          if (order.isLong) { 
  360              ERC20(order.baseAsset).safeTransferFrom(  

  489:         emit ExercisedOrder(orderHash, floorAssetTokenIds, order);
  490  
  491          /* ~~~ INTERACTIONS ~~~ */
  492  
  493          if (order.isCall) {
  494              // -- exercising a call option
  495  

  576:         emit WithdrawOrder(orderHash, order);
  577  
  578          /* ~~~ INTERACTIONS ~~~ */
  579  
  580          // transfer strike to owner if put is expired or call is exercised
  581          if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
  582              // send the fee to the admin/DAO if fee is greater than 0%

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

PuttyV2.sol:548:    function withdraw(Order memory order) public {
PuttyV2.sol:890:    function encodeERC721Assets(ERC721Asset[] memory arr)  
PuttyV2.sol:912:    function domainSeparatorV4() public view returns (bytes32) {

Overview

Table of Contents:

• 1. Unchecking arithmetics operations that can't underflow/overflow
• 2. <array>.length should not be looked up in every loop of a for-loop
• 3. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 4. Increments/decrements can be unchecked in for-loops
• 5. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied
• 6. Use Custom Errors instead of Revert Strings to save Gas

1. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here:

PuttyV2.sol:591:                order.strike - feeAmount

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

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, Consider storing the array's length in a variable before the for-loop, and use this new variable instead:

PuttyV2.sol:653:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:697:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:716:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:736:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:750:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:763:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:781:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:801:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:871:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:895:        for (uint256 i = 0; i < arr.length; i++) {

3. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

PuttyV2.sol:653:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:697:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:716:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:736:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:750:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:763:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:781:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:801:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:871:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:895:        for (uint256 i = 0; i < arr.length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

4. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Affected code:

PuttyV2.sol:653:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:697:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:716:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:736:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:750:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:763:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:781:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:801:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:871:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:895:        for (uint256 i = 0; i < arr.length; i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

5. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied

If the optimizer is enabled, this finding isn't true anymore If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

PuttyV2.sol:583:            uint256 feeAmount = 0;
PuttyV2.sol:653:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:697:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:716:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:736:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:750:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:763:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:781:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:801:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:871:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:895:        for (uint256 i = 0; i < arr.length; i++) {

Consider removing explicit initializations for default values.

6. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 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.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Consider replacing all revert strings with custom errors in the solution.

PuttyV2.sol:231:        require(_weth != address(0), "Unset weth address");
PuttyV2.sol:258:        require(_fee < 30, "fee must be less than 3%");
PuttyV2.sol:295:        require(
PuttyV2.sol:305:        require(!cancelledOrders[orderHash], "Order has been cancelled"); 
PuttyV2.sol:308:        require(
PuttyV2.sol:315:        require(order.duration < 10_000 days, "Duration too long"); 
PuttyV2.sol:318:        require(block.timestamp < order.expiration, "Order has expired"); 
PuttyV2.sol:321:        require(order.baseAsset.code.length > 0, "baseAsset is not contract"); 
PuttyV2.sol:325:            ? require(
PuttyV2.sol:329:            : require(
PuttyV2.sol:369:                require(
PuttyV2.sol:404:                require(msg.value == order.strike, "Incorrect ETH amount sent"); 
PuttyV2.sol:457:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner"); 
PuttyV2.sol:460:        require(order.isLong, "Can only exercise long positions"); 
PuttyV2.sol:463:        require(
PuttyV2.sol:470:            ? require(
PuttyV2.sol:474:            : require(
PuttyV2.sol:500:                require(msg.value == order.strike, "Incorrect ETH amount sent");
PuttyV2.sol:552:        require(!order.isLong, "Must be short position");
PuttyV2.sol:557:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
PuttyV2.sol:563:        require(
PuttyV2.sol:621:        require(msg.sender == order.maker, "Not your order");
PuttyV2.sol:645:        require(orders.length == signatures.length, "Length mismatch in input");
PuttyV2.sol:646:        require(
PuttyV2.sol:701:            require(token.code.length > 0, "ERC20: Token is not contract");
PuttyV2.sol:702:            require(tokenAmount > 0, "ERC20: Amount too small");
PuttyV2.sol:924:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");
PuttyV2Nft.sol:13:        require(to != address(0), "INVALID_RECIPIENT");
PuttyV2Nft.sol:14:        require(_ownerOf[id] == address(0), "ALREADY_MINTED");
PuttyV2Nft.sol:29:        require(from == _ownerOf[id], "WRONG_FROM");
PuttyV2Nft.sol:30:        require(to != address(0), "INVALID_RECIPIENT");
PuttyV2Nft.sol:31:        require(
PuttyV2Nft.sol:46:        require(owner != address(0), "ZERO_ADDRESS");