Putty contest - xiaoming90's results

Lines of code

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

Vulnerability details

Note: This issue only affects the "Short Put" workflow

Vulnerability Details

An order could contain the following:

• An invalid ERC20 asset address within the ERC20Asset[] array
• An invalid ERC721 NFT address within the ERC721Asset[] array
• An invalid floorToken address within the floorTokens[] array

This issue only affects the "Short Put" workflow, where an order maker creates a short put order, and an order taker attempts to fill the short put order. This is because during the "Short Put" workflow:

• The malicious order maker (attacker) does not have to transfer any ERC20 asset, ERC721 NFT or floorToken when the order is being filled OR option is being exercised. Thus, this issue will not occur on the attacker-side.
• The order taker (victim) does not have to transfer any ERC20 asset, ERC721 NFT or floorToken when filling the malicious order. Thus, the victim will only be aware of this issue when the victim tried to exercise the option, which is too late as the victim has already transferred the premium to the attacker.

Proof-of-Concept

The following illustrates an example of how an attacker could specify an invalid ERC20 Asset within an order to cause the victim to loss funds. The step is the same for an invalid ERC721 NFT or floorToken, thus it is omitted for brevity.

Note: Fee is ignored for simplicity.

1. Alice (Attacker) create a Short Put order with the following configuration:

Strike Price = 100 ETH

Premium = 5 ETH

isLong = False

isCall = False

baseAsset = WETH

ERC20Asset[] = [0xDAI, 0xUSDC, 0xNONEXIST] // The last address is an invalid erc20 token address that does not exist

2. Bob (Victim) is not aware that the one of the ERC20 Assets within Alice's order is invalid. Thus, he decided to fill Alice's order by calling PuttyV2.fillOrder. .
3. Thus, Bob send 5 ETH (premium) to Alice, and Alice transfer 100 ETH (strike price) to PuttyV2 contract for escrow.
4. At this point, Bob holds a Long Put order, while Alice holds a Short Put order.
5. At certain point of time, Bob decided to exercise his Long Put option by calling PuttyV2.exercise function. In layman term, Bob wanted to exercise his option to sell the ERC20 assets at the strike price at this point of time.
6. However, Bob has some issues now. He is able to get the 0xDAI and 0xUSDC ERC20 assets from the open market, but there is no way for Bob to get the 0xNONEXIST ERC20 asset anywhere as it does not exist. Thus, Bob could not exercise his Long Put option because according to the specification/design, he need hold all the 3 ERC20 assets (0xDAI, 0xUSDC, and 0xNONEXIST) in his account to order to exercise his option. Thus, the PuttyV2.exercise function will revert due to missing 0xNONEXIST ERC20 asset in Bob account.
7. When the Bob's Long Put option expired, Alice proceeds to reclaim back the 100 ETH that she previously escrowed within the PuttyV2 contract.
8. Alice earns the 5 ETH premium

Alice could initiate multiple malicious orders at a time to 'fish' for victims in Putty Protocol. As long as there is someone willing to fill her order, she will earn the premium in a risk-free environment because the order takers can never exercise their option.

Alice could make the orders more attractive by configuring the order in a manner that will attract potential takers.

Impact

The order taker (victim) would lost their funds as they paid for an option that cannot be exercised.

Recommended Mitigation Steps

It is recommend that the PuttyV2.fillOrder function should perform a sanity check against the "Short Put" order to be filled to ensure the following requirements are in place before filling an order to reduce the risk:

• All ERC20 assets specified within the ERC20Asset[] array should point to a valid contract and should be valid (e.g. tokenAmount > 0)
• All ERC721 NFTs specified within the ERC721Asset[] array should point to a valid contract and should be valid (e.g. tokenId > 0)
• All floorTokens specified within the floorTokens[] array should point to a valid contract

If possible, a whitelisting mechanism should be implemented to only allow approved tokens vetted by Putty to be traded within Putty. The above validation is a best-effort approach to check whether a contract exists at the specified address, but it cannot guarantee that the contract is a valid ERC20 or ERC721 contract, thus it cannot totally eliminate the risk.

The following sanity check could be implemented within the PuttyV2.fillOrder to meet the above-mentioned requirements.

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
    ..SNIP..
    // filling short put
    if (!order.isLong && !order.isCall) {
    		// verify that all ERC20 assets, ERC721 NFTs and floorTokens within an order is valid.
				sanityCheckOnShortPutOrder(order.erc20Assets, order.erc721Assets, order.floorTokens);
    }
    ..SNIP..
}

// verify that all ERC20 assets, ERC721 NFTs and floorTokens within an order is valid.
function sanityCheckOnShortPutOrder(ERC20Asset[] memory assets, ERC721Asset[] memory nfts, address[] memory floorTokens) {
    for (uint256 i = 0; i < assets.length; i++) {
        require(assets[i].token.code.length > 0, "ERC20: Token is not contract");
        require(assets[i].tokenAmount > 0, "ERC20: Amount too small");
    }

    for (uint256 i = 0; i < nfts.length; i++) {
        require(nfts[i].token.code.length > 0, "ERC721: Token is not contract");
        require(nfts[i].tokenId > 0, "ERC721: Invalid token ID");
    }
    
    for (uint256 i = 0; i < floorTokens.length; i++) {
        require(floorTokens[i].code.length > 0, "FloorToken is not contract");
    }
    
    // Specify other sanity checks if needed
}

Although client-side or off-chain might have already implemented such validations, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented in the on-chain contracts.

Additionally, Putty should inform the order taker about the following:

• The risks of an invalid ERC20 asset, ERC721 NFT or floorToken within an order
• The need for the order taker to perform the necessary due diligence against all the ERC20 assets, ERC721 NFTs and floorTokens within an order before filling it.

Lines of code

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

Vulnerability details

Note: This issue only affects the "Short Put" workflow

Vulnerability Details

An order could contain an unbounded number of floorTokens, erc20Assets and/or erc721Assets.

Although multiple unbounded array issues exist within the in-scope contracts, this issue is different in a way that it demostrated that it will result in loss of funds for the users (long option holders) as they will not be able to exercise their option as shown in the Proof-of-Concept section.

This issue only affects the "Short Put" workflow, where an order maker creates a short put order, and an order taker attempts to fill the short put order. This is because during the "Short Put" workflow:

• The malicious order maker (attacker) does not have to transfer any floorTokens, erc20Assets and/or erc721Assets when the order is being filled or option is being exercised. Thus, this issue will not occur on the attacker-side.
• The order taker (victim) does not have to transfer any floorTokens, erc20Assets and/or erc721Assets when filling the malicious order. Thus, the victim will only be aware of this issue when the victim tried to exercise the option, which is too late as the victim has already transferred the premium to the attacker.

Proof-of-Concept

The following illustrate an example of how an attacker could make use of the unbounded erc20Assets to cause the victim to loss funds.

Note: Fee is ignored for simplicity.

1. Alice (Attacker) create a Short Put order with the following configuration:

Strike Price = 100 ETH

Premium = 5 ETH

isLong = False

isCall = False

baseAsset = WETH

ERC721Asset[] = an array of 1000 NFTs (or any large number of NFTs that cause out-of-gas error)

2. Bob (Victim) decided to fill Alice's order by calling PuttyV2.fillOrder. Thus, Bob send 5 ETH (premium) to Alice, and Alice transfer 100 ETH (strike price) to PuttyV2 contract for escrow.

3. At this point, Bob holds a Long Put order, while Alice holds a Short Put order.

4. At certain point of time, Bob decided to exercise his Long Put option by calling PuttyV2.exercise function. In layman term, Bob wanted to exercise his option to sell NFTs at the strike price at this point of time.

5. Thus, the PuttyV2 contract will attempt to transfer 100 ETH (strike price) escrowed within the contract to Bob, and the PuttyV2 contract will "pull" 1000 NFTs from Bob.

6. However, due to large amount of NFTs that need to be transferred from Bob to PuttyV2 contract. Thus, the PuttyV2.exercise function will always revert due to out-of-gas error. As a result, there is no way for Bob to exercise his Long Put option.

7. When the Bob's Long Put option expired, Alice proceeds to reclaim back the 100 ETH that she previously escrowed within the PuttyV2 contract.

8. Alice earns the 5 ETH premium

Alice could initiate multiple malicious orders at a time to 'fish' for victims in Putty Protocol. As long as there is someone willing to fill her order, she will earn the premium in a risk-free environment because the order takers can never exercise their option.

Alice could make the orders more attractive by configuring the order in a manner that will attract potential takers.

Impact

The order taker (victim) would lost their funds as they paid for an option that cannot be exercised.

Recommended Mitigation Steps

It is recommended to restrict the number of floorTokens, erc20Assets and erc721Assets within an order to a upper limit (e.g. 10).

Although client-side or off-chain might have already verified that the number of floorTokens, erc20Assets and erc721Assets do not exceed a certain limit within an order, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented on the on-chain contracts.

Lines of code

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

Vulnerability details

Proof-of-Concept

The PuttyV2.exercise is a payable function that allows users to transfer native ETH when exercising an order. The native ETH is only applicable when the baseAsset is equal to WETH AND during the following scenarios:

• When transferring strike from exerciser to PuttyV2 contract

In all other circumstances, native ETH transfer is not applicable within the PuttyV2.exercise function. The following table summarises whether or not the two (2) key workflows within the PuttyV2.exercise function requires native ETH transfer:

Thus, it is possible that the users accidentially attached non-zero Ether value to the transaction when calling PuttyV2.exercise function when executing the Long Put workflow. Thus, the user's Ether will be locked within the contract without the ability to retrieve it.

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

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

Impact

User's Ether locked in the contract.

Recommended Mitigation Steps

It is recommended to reverts if msg.value > 0 for workflows or scenarios that Native ETH is not expected or required.

When the order.baseAsset is not equal to WETH AND ``msg.value > 0 , it should reverts.

Additionally, when the user attempts to exercise a "Long Put" order, reverts if msg.value > 0 as exercising a "Long Put" does not expect the users to send any native ETH.

function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
	// If baseAsset is not WETH, there shouldn't be any native ETH attached
    if (order.baseAsset != weth && msg.value > 0) {
    	revert();
    }
    // If excercising long put, there shouldn't be any native ETH attached
    if (order.isLong && !order.isCall && msg.value > 0) {
    	revert();
    }
    ..SNIP..

Lines of code

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

Vulnerability details

Proof-of-Concept

When users withdraw their strike escrowed in Putty contract, Putty will charge a certain amount of fee from the strike amount. The fee will first be sent to the contract owner, and the remaining strike amount will then be sent to the users.

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

function withdraw(Order memory order) public {
	..SNIP..

	// transfer strike to owner if put is expired or call is exercised
	if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
		// send the fee to the admin/DAO if fee is greater than 0%
		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);

		return;
	}
	..SNIP..
}

There are two methods on how the owner can deny user from withdrawing their strike amount from the contract

Method #1 - Set the owner() to zero address

Many of the token implementations do not allow transfer to zero address (Reference). Popular ERC20 implementations such as the following Openzeppelin's ERC20 implementation do not allow transfer to zero address, and will revert immediately if the to address (recipient) points to a zero address during a transfer.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5fbf494511fd522b931f7f92e2df87d671ea8b0b/contracts/token/ERC20/ERC20.sol#L226

function _transfer(
    address from,
    address to,
    uint256 amount
) internal virtual {
    require(from != address(0), "ERC20: transfer from the zero address");
    require(to != address(0), "ERC20: transfer to the zero address");

    _beforeTokenTransfer(from, to, amount);

    uint256 fromBalance = _balances[from];
    require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
    unchecked {
        _balances[from] = fromBalance - amount;
        // Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
        // decrementing then incrementing.
        _balances[to] += amount;
    }

    emit Transfer(from, to, amount);

    _afterTokenTransfer(from, to, amount);
}

It is possible for the owner to transfer the ownership to a zero address, thus causing the fee transfer to the contract owner to always revert. When the fee transfer always reverts, no one can withdraw their strike amount from the contract.

This issue will affect all orders that adopt a baseAsset that reverts when transferring to zero address.

Method #2 - If baseAsset is a ERC777 token

Note: owner() could point to a contract or EOA account. By pointing to a contract, the contract could implement logic to revert whenever someone send tokens to it.

ERC777 contains a tokensReceived hook that will notify the recipient whenever someone sends some tokens to the recipient .

Assuming that the baseAsset is a ERC77 token, the recipient, which is the owner() in this case, could always revert whenever PuttyV2 contract attempts to send the fee to recipient. This will cause the withdraw function to revert too. As a result, no one can withdraw their strike amount from the contract.

This issue will affect all orders that has ERC777 token as its baseAsset.

Impact

User cannot withdraw their strike amount and their asset will be stuck in the contract.

Recommended Mitigation Steps

It is recommended to adopt a withdrawal pattern for retrieving owner fee.

Instead of transferring the fee directly to owner address during withdrawal, save the amount of fee that the owner is entitled to in a state variable. Then, implement a new function that allows the owner to withdraw the fee from the PuttyV2 contract.

Consider the following implementation. In the following example, there is no way for the owner to perform denial-of-user because the outcome of the fee transfer (succeed or fail) to the owner will not affect the user's strike withdrawal process.

This will give users more assurance and confidence about the security of their funds stored within Putty.

mapping(address => uint256) public ownerFees;

function withdraw(Order memory order) public {
	..SNIP..
    // transfer strike to owner if put is expired or call is exercised
    if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
        // send the fee to the admin/DAO if fee is greater than 0%
        uint256 feeAmount = 0;
        if (fee > 0) {
            feeAmount = (order.strike * fee) / 1000;
            ownerFees[order.baseAsset] += feeAmount
        }

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

        return;
    }
    ..SNIP..
}

function withdrawFee(address baseAsset) public onlyOwner {
	uint256 _feeAmount = ownerFees[baseAsset];
	ownerFees[baseAsset] = 0;
	ERC20(baseAsset).safeTransfer(owner(), _feeAmount);
}

Lines of code

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

Vulnerability details

Proof-of-Concept

The PuttyV2.fillOrder function will validate that the order duration is less than 10,000 days. However, it does not check that order duration is equal to zero. Thus, it is possible for a malicious order maker to create an order with order.duration equal to zero. This will cause the order to expired immediately after being filled by a taker.

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

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
   ..SNIP..
    // check duration is valid
    require(order.duration < 10_000 days, "Duration too long");
    ..SNIP..
    // save the long position expiration
    positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;
    ..SNIP..
}

An attacker (Alice) could perform the following:

1. Alice (order maker) create an "Short Call" order with order.duration set to zero.
2. Bob (order taker) make a mistake by filling Alice's order, and send the premium to Alice. Upon filling Alice's order, Bob's long call option expires immediately, and there is no chance for him to exercise his option.
3. Alice gains the premium in a risk-free environment, and get to keep her assets.

Impact

The order taker (victim) would lost their funds as they paid for an option that cannot be exercised because the option expired immediately after being bought.

Recommended Mitigation Steps

It is recommended to implement additional validation to ensure that the order.duration is not zero or set to short period of time (e.g. 5 minute).

An option that expires immediately or expires within a short period of time (e.g. 10 seconds or 5 minutes) does not have much value, and thus it should not be allowed within Putty. Order makers who create such orders are likely to be malicious user who exploits this flexibility to 'trick' or 'fish' the users into filling their order to obtain the premium, thus causing harm to the community.

1. Codebase Summary & Key Improvement Opportunities

Key Features

The in-scope system was found to be low in complexity, with the key features being as follows:

• fillOrder - Fills an offchain order
• exercise - Exercise a long order
• withdraw - Withdraws the assets from a short order
• cancel - Cancels an order which prevents it from being filled in the future

Code Quality and Test Coverage

In sumamry, the code quality of the PuttyV2 is high. The codes were also found to be well-documented and team took the efforts to document the NatSpec for all the functions within the PuttyV2 contract. However, there are still some room of improvement as NatSpec was not documented for the PuttyV2Nft contract. For completeness and readability, it is recommended to document the NatSpec for all the functions in the contracts where feasible.

To futher improve readability of the codes, additional helper functions could be implemented. Refer to the "4.6 Code Can Be Refactored To Be More Readable" issue.

Test coverage was found to be high. All the key features have been covered in the test. However, periphery logic functions such as batchFillOrder and acceptCounterOffer that are exposed to the public users were not included in the tests. It is recommended to write proper tests for all functions.

Unexpected Token Behaviors

The system did not implement a whitelisting mechanism to ensure that only approved tokens are allowed to be traded within Putty. Thus, users could specify any tokens within an order/option. This permissionless approach increases the attack surface of the system and exposes the system to various attack vectors. Some tokens might be malicious, some tokens might contain hooks, and some tokens might not work as intended. Thus, it is challenging for Putty to guard against all kinds of vulnerabilites that arise due to the need to support large number of tokens. If possible, it is recommended to only allow approved tokens that have been vetted and reviewed by Putty to be traded within the system to minimize the risks during the initial stage. Once the system is more stable, the team can progressively open up to more tokens.

Re-entrancy Risks

The key features (e.g. fillOrder, exercise) were found to be following the "Checks Effects Interactions" pattern rigorously, which help to prevent any possible re-entrancy attack. However, further improvement can be made to guard against future re-entrancy attack. As the key features make many external contract calls via token transfers, the risks of re-entrancy attack is significantly higher for Putty compared to other protocols. Thus, it is prudent to implement additional reentrancy prevention wherever possible by utilizing the nonReentrant modifier from Openzeppelin Library to block possible re-entrancy as a defense-in-depth measure.

Out-of-gas/Revert Risks

The order contains many arrays such as whitelist, floorTokens, erc20Assets, and erc721Assets. It was found that the contract did not place an upper limit on the number of elements that can be stored within the array, and proceed to loop through all the elements within an array in many parts of the codes. This might cause out-of-gas error to happen and cause a revert, thus causing the feature to be unusable in certain scenarios. It is recommended for the team to review each of the loop structures involving an array within the contract to determine if it is possible to place an upper limit.

Authorsation Controls

Robust authorisation controls have been implemented for all the key features to ensure that only authorised and correct actors could call the functions. For instance, only owner of long position could call the exercise function and only owner of short position could call the withdraw function. No authorisation issues were observed during the contest.

2. Summary Of Findings

The following is a summary of the low and non-critical findings observed during the contest.

3. Low Risk Issues

3.1 Lack Of Reentrancy Guards On External Functions

Description

The following external functions within the PuttyV2 contract contain function calls (e.g. safeTransferFrom, safeTransfer) that pass control to external contracts. Additionally, if ERC777 tokens are being used within an order, it contains various hooks that will pass the execution control to the external party.

Thus, it might allow an malicious external contract to re-enter to the contract.

• PuttyV2.fillorder
• PuttyV2.exercise
• PuttyV2.withdraw
• PuttyV2.batchFillOrder
• Putty.acceptCounterOffer

No re-entrancy attacks that could lead to loss of assets were observed during the assessment. Thus, this issue is marked as Low.

The following shows examples of function call being made to an external contract

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#L436

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

Recommendation

It is recommended to follow the good security practices and apply necessary reentrancy prevention by utilizing the nonReentrant modifier from Openzeppelin Library to block possible re-entrancy.

3.2 Discontinuity in Exercise Period

Description

The position can be exercised if current block timestamp is less than the position's expiration.

The position can be withdrawed if current block timestamp is greater than the position's expiration

However, when current block timestamp is equal to the position's expiration (block.timestamp == positionExpirations), the state is unknown (cannot be exercised or withdraw)

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

function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
    ..SNIP..
    // check position has not expired
    require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
    ..SNIP..
}

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

function withdraw(Order memory order) public {
    ..SNIP..
    // check long position has either been exercised or is expired
    require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
    ..SNIP..
}

Recommendation

Allow the user to withdraw the position upon expiration.

function withdraw(Order memory order) public {
    ..SNIP..
    // check long position has either been exercised or is expired
    require(block.timestamp >= positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
    ..SNIP..
}

3.3 Insufficient Input Validation

Description

The PuttyV2.fillOrder function does not validate that the msg.sender (order taker) is the same as the order maker, which might potentially lead to unwanted behaviour within the system. Order taker should not be the same as order maker under any circumstances.

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

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

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

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

Recommendation

Implement the necessary check to ensure that order taker is not the same as order maker.

require(msg.sender != order.maker, "Invalid order taker");

3.4 Order Cannot Be Filled Due To Unbounded Whitelist Within An Order

Description

An order can contain large number of addresses within the whitelist array of an order.

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

struct Order {
    address maker;
    bool isCall;
    bool isLong;
    address baseAsset;
    uint256 strike;
    uint256 premium;
    uint256 duration;
    uint256 expiration;
    uint256 nonce;
    address[] whitelist;
    address[] floorTokens;
    ERC20Asset[] erc20Assets;
    ERC721Asset[] erc721Assets;
}

When the PuttyV2.fillOrder function is called, it will attempt to check if the caller is whitelisted by looping through the order.whitelist array. However, if order.whitelist array contains large number of addresses, it will result in out-of-gas error and cause a revert. Thus, this order can never be filled.

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

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) { // @audit-issue no re-entrancy guard
	..SNIP..
    // check msg.sender is allowed to fill the order
    require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
	..SNIP..
}

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

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

    return false;
}

Recommendation

It is recommended to restrict the number of whitelisted addresses within an order to a upper limit (e.g. 30).

Although client-side or off-chain might have already verified that the number of whitelisted addresses do not exceed a certain limit within an order, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented on the on-chain contracts.

3.5 Order Cannot Be Filled Due To Unbounded floorTokens, ERC20Asset Or ERC721Asset Within An Order

Description

An order can contain large number of tokens within the floorTokens, ERC20Asset or ERC721Asset arrays of an order.

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

struct Order {
    address maker;
    bool isCall;
    bool isLong;
    address baseAsset;
    uint256 strike;
    uint256 premium;
    uint256 duration;
    uint256 expiration;
    uint256 nonce;
    address[] whitelist;
    address[] floorTokens;
    ERC20Asset[] erc20Assets;
    ERC721Asset[] erc721Assets;
}

When the PuttyV2.fillOrder function is called, it will attempts to loop through all the floorTokens, ERC20Asset or ERC721Asset arrays of an order to transfer the required assets to PuttyV2 contract from the order maker or taker.

The _transferERC20sIn, _transferERC721sIn, _transferFloorsIn attempt to loop through all the tokens within the array. However, if array contains large number of tokens, it will result in out-of-gas error and cause a revert. Thus, this order can never be filled.

Following is an example of the vulnerable function.

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

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

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

        ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
    }
}

Recommendation

It is recommended to restrict the number of tokens within the floorTokens, ERC20Asset or ERC721Asset arrays of an order. (e.g. Maximum of 10 tokens)

Although client-side or off-chain might have already verified that the number of tokens do not exceed a certain limit within an order, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented on the on-chain contracts.

3.6 Order Can Be Cancelled Even After Being Filled

Description

Once an order has been filled, no one should be able to cancel the order or mark the order as Cancelled.

The following code shows that the order maker can change the status of the order to Cancelled at any point of time.

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

/**
    @notice Cancels an order which prevents it from being filled in the future.
    @param order The order to cancel.
 */
function cancel(Order memory order) public {
    require(msg.sender == order.maker, "Not your order");

    bytes32 orderHash = hashOrder(order);

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

    emit CancelledOrder(orderHash, order);
}

Although changing the status of an order to Cancelled after it has been filled does not cause any lost of funds at the later stages (e.g. when exercising or withdrawing), it might cause unnecessary confusion to the users as it does not accurately reflect the status of an order on-chain.

Users might fetch the status of an order directly from the cancelledOrders mapping or poll the on-chain for emitted event, and come to a wrong conclusion that since the order has been cancelled, it has not been filled.

Recommendation

It is recommended to update the cancel function to only allow order maker to call this function only if an order has not been filled.

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

    bytes32 orderHash = hashOrder(order);
    
    // If an order has been filled, the positionExpirations[orderHash] will be populated.
    require(positionExpirations[orderHash] == 0, "Order has already been filled. Cannot cancel.")

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

    emit CancelledOrder(orderHash, order);
}

3.7 No Check if onERC721Received Is Implemented

Description

The PuttyV2.fillOrder will mint a long position NFT and short position NFT to the order maker and taker. When minting a NFT, the function does not check if a receiving contract implements onERC721Received().

The intention behind this function is to check if the address receiving the NFT, if it is a contract, implements onERC721Received(). Thus, there is no check whether the receiving address supports ERC-721 tokens and position could be not transferrable in some cases.

Following shows that _mint is used instead of _safeMint.

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

function fillOrder(
	Order memory order,
	bytes calldata signature,
	uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
	..SNIP..
	// create long/short position for maker
	_mint(order.maker, uint256(orderHash));

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

Recommendation

Consider using _safeMint instead of _mint.

4. Non-Critical Issues

4.1 Omissions in events

Description

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

Instance #1 - Missing Old Value

When setting a new baseURI and fee, only the new value is emitted within the event.

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

function setBaseURI(string memory _baseURI) public payable onlyOwner {
    baseURI = _baseURI;
    emit NewBaseURI(_baseURI);
}

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

function setFee(uint256 _fee) public payable onlyOwner {
    require(_fee < 30, "fee must be less than 3%");
    fee = _fee;
    emit NewFee(_fee);
}

The events should include the new value and old value where possible.

4.2 Draft OpenZeppelin Dependencies

Description

The PuttyV2 contract utilised draft-EIP712 , an OpenZeppelin contract. This contract is still a draft and is not considered ready for mainnet use. OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.

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

import "openzeppelin/utils/cryptography/SignatureChecker.sol";
import "openzeppelin/utils/cryptography/draft-EIP712.sol";
import "openzeppelin/utils/Strings.sol";

Recommendation

Ensure the development team is aware of the risks of using a draft contract or consider waiting until the contract is finalised.

Otherwise, make sure that development team are aware of the risks of using a draft OpenZeppelin contract and accept the risk-benefit trade-off.

4.3 Insufficient Tests

Description

It is crucial to write tests with possibly 100% coverage for smart contract systems.

The following functions were found to be not included in the test cases:

• PuttyV2.batchFillOrder - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L546
• PuttyV2.acceptCounterOffer - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L573

Recommendation

It is recommended to write proper tests for all possible code flows and specially edge cases

4.4 Owner Can Renounce Ownership

Description

Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The Openzeppelin's Ownable used in PuttyV2 contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

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

/**
    @title PuttyV2
    @author out.eth
    @notice An otc erc721 and erc20 option market.
 */
contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Ownable {

Recommendation

We recommend to either reimplement the function to disable it or to clearly specify if it is part of the contract design

4.5 Consider two-phase ownership transfer

Description

Admin calls Ownable.transferOwnership function to transfers the ownership to the new address directly. As such, there is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without a owner.

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

contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Ownable {

Recommendation

Consider implementing a two step process where the admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated EOA account is a valid and active account.

4.6 Code Can Be Refactored To Be More Readable

Description

In many parts of the PuttyV2 contract, it uses the following conditions to check the type of the order being passed into the function:

• order.isLong && order.isCall (equal to long call)
• order.isLong && !order.isCall (equal to long put)
• order.!isLong && order.isCall (equal to short call)
• order.!isLong && order.!isCall (equal to short put)

These affect the readability of the codes as the readers have to interpret the condition to determine if it is a "long call", "long put", "short call" or "short put". This might increase the risk of mistakes in the future if new developer works on the contracts.

Recommendation

Consider implementing the following functions to improve readability:

• isLongCall(Order order) public view returns (bool)
• isLongPut(Order order) public view returns (bool)
• isShortCall(Order order) public view returns (bool)
• isShortPut(Order order) public view returns (bool)

4.7 Inconsistent use of named return variables

Description

There is an inconsistent use of named return variables in the PuttyV2 contract

Some functions return named variables, others return explicit values.

Following function return explicit value

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

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

    return false;
}

Following function return return a named variable

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

function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
    // use decode/encode to get a copy instead of reference
    Order memory oppositeOrder = abi.decode(abi.encode(order), (Order));

    // get the opposite side of the order (short/long)
    oppositeOrder.isLong = !order.isLong;
    orderHash = hashOrder(oppositeOrder);
}

Recommendation

Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

4.8 Unused imports

Description

To improve readability and avoid confusion, consider removing the following unused imports:

In the PuttyV2Nft contract:

• openzeppelin/utils/Strings.sol

Note that the Strings.sol has already been imported in PuttyV2 contract. Thus, this import can be safely removed.

Within the PuttyV2Nft contract, it does not use any of the functions from Strings.sol.

Recommendation

Consider removing the unused import if it is not required.

4.9 Incorrect functions visibility

Description

Whenever a function is not being called internally in the code, it can be easily declared as external, saving also gas. While the entire code base have explicit visibilities for every function, some of them can be changed to be external.

Following are some the functions that can be changed to be external

• PuttyV2.fillorder
• PuttyV2.exercise
• PuttyV2.withdraw
• PuttyV2.batchFillOrder
• Putty.acceptCounterOffer

Recommendation

Review the visibility of the affected functions and change visibility of these functions to external.

4.10 NatSpec Is Missing

Description

NatSpec if missing for the following function

• PuttyV2Nft._mint - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L11
• PuttyV2Nft.transferFrom - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L21
• PuttyV2Nft.balanceOf - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L40

Recommendation

Implement NatSpec for all functions.