Putty contest - reassor's results

Low Risk

• 1. Missing airdrops/benefits from locked ERC20/ERC721 tokens
• 2. Attempt to send 0 fee for small strike price
• 3. ERC721 token used as order's base assets
• 4. ERC721 tokens used for order's erc20 assets
• 5. Putty's NFT can be used as order's erc721 assets or floor tokens
• 6. Critical address change
• 7. Lack of support for tokens with fee - RISK ACCEPTED
• 8. Lost of support for rebase tokens - RISK ACCEPTED

Non-Critical

• 9. Complex code
• 10. Missing/incomplete natspec comments

1. Missing airdrops/benefits from locked ERC20/ERC721 tokens

Risk

Low

Impact

Tokens ERC20/ERC721 are locked in the contract:

• for orders duration - short call and long call orders
• between exercise and withdraw - short put and short call orders

The issue is that contract PuttyV2 does not have functionality for withdrawing excess of ERC20/ERC721 tokens which means that all potential airdrops/benefits for holding given tokens are getting locked in the contract.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to track total amounts of currently deposited tokens and allow owner to withdraw the excess, that could be later used for benefiting users of PuttyV2.

2. Attempt to send 0 fee for small strike price

Risk

Low

Impact

Contract PuttyV2 charges feeAmount from order.strike that is calculated based on defined fee.

Following code is responsible for charging fee:

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

In case the order.strike is small - order.strike * fee < 1000 it will get rounded down to 0 and then function safeTransfer will attempt to send 0 tokens to the owner.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add additional check before safeTransfer invocation to make sure that feeAmount is not equal to 0.

3. ERC721 token used as order's base asset

Risk

Low

Impact

It is possible to use ERC721 tokens as order's base asset which will work fine for fillOrder since it is using safeTransferFrom that is also implemented for ERC721 tokens. The issue is that these kind of orders will be properly filled but it will not possible to exercise/withdraw them since the contract will try to use safeTransfer function that does not exist in ERC721.

Proof of Concept

PuttyV2.sol:

• order.premium used as tokenId - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L324
• order.premium used as tokenId - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L338
• order.strike used as tokenId - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L344
• order.strike used as tokenId - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L360

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check in fillOrder (via ERC165 interface) to make sure that order's base asset is not ERC721 token.

4. ERC721 tokens used for order's erc20 assets

Risk

Low

Impact

It is possible to use ERC721 tokens as order's ERC20 assets erc20Assets which will work fine for transferring assets to the contract since safeTransferFrom is being used in _transferERC20sIn and it is also implemented by ERC721 tokens. The issue is that orders crafted like that will fail for any trigger of _transferERC20sOut that is using safeTransfer that is not implemented in ERC721 tokens.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check in _transferERC20sIn (via ERC165 interface) to make sure that order's base asset is not ERC721 token.

5. Putty's NFT can be used as order's erc721 assets or floor tokens

Risk

Low

Impact

It is possible to use PuttyV2 NFT (long/short positions) as order's erc721Assets or floorTokens. This might lead to unpredictable scenarios where malicious orders can be filled but cannot be properly processed via exercise or withdraw.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add checks in fillOrder to make sure that none of erc721Assets and floorTokens are Putty's contract address.

6. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing ownership.

7. Lack of support for tokens with fee - RISK ACCEPTED

Risk

Low

Impact

Contract PuttyV2 does not support tokens with fees.

Contract PuttyV2 does not handle ERC20 tokens that charge fee on their transfers. Implementation of such a tokens does not transfer exact amount provided to transfer() but part of it is charged as a fee, burned or used in some other way. This leads to incorrect accounting and effectively to loss of funds.

Since the team accepted that the contract will not be supporting Rebase and fee-on-transfer tokens the severity has been downgraded to low.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add support for ERC20 tokens with built-in fees.

8. Lost of support for rebase tokens - RISK ACCEPTED

Impact

Contract PuttyV2 does not support rebase tokens.

Rebasing tokens are tokens that have each holder’s balanceof() increase over time. Aave aTokens are an example of such tokens.

Since the team accepted that the contract will not be supporting Rebase and fee-on-transfer tokens the severity has been downgraded to low.

Proof of Concept

PuttyV2.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add support for ERC20 tokens wth rebase functionality by tracking total amounts currently deposited and allow owner to withdraw excess.

9. Complex code

Risk

Non-Critical

Impact

Contract PuttyV2 implements effectively options logic in just 3 complex functions:

• fillOrder
• exercise
• withdraw

While the approach is very smart and neat it makes code less readable and thus prone to errors.

Proof of Concept

• fillOrder functions - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L268-L380
• exercise function - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L389-L458
• withdraw function - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L466-L520

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to distribute logic of listed functions into multiple internal/private functions.

10. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Contracts PuttyV2NFT and PuttyV2 are missing some natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

PuttyV2NFT.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L9
• Missing natspec - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L21
• Missing natspec - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L40

StakingPuttyV2.sol:

• Missing natspec for constructor - https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L209-L213

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

List

• 1. Cache array length outside of loop
• 2. Use custom errors instead of revert strings to save gas
• 3. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1
• 4. Obsolete overflow/underflow check
• 5. No need to explicitly initialize variables with default values
• 6. Use != 0 instead of > 0 for unsigned integer comparison

1. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Recommended Mitigation Steps

It is recommended to cache the array length outside of for loop.

2. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

• PuttyV2.sol
• PuttyV2Nft.sol

Recommended Mitigation Steps

It is recommended to add custom errors to listed contracts.

3. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Recommended Mitigation Steps

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

4. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Recommended Mitigation Steps

It is recommended to wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }

5. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

PuttyV2.sol:497:            uint256 feeAmount = 0;
PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Recommended Mitigation Steps

It is recommended to remove explicit initializations with default values.

6. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

PuttyV2.sol:293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
PuttyV2.sol:327:            if (weth == order.baseAsset && msg.value > 0) {
PuttyV2.sol:351:            if (weth == order.baseAsset && msg.value > 0) {
PuttyV2.sol:427:            if (weth == order.baseAsset && msg.value > 0) {
PuttyV2.sol:498:            if (fee > 0) {
PuttyV2.sol:598:            require(token.code.length > 0, "ERC20: Token is not contract");
PuttyV2.sol:599:            require(tokenAmount > 0, "ERC20: Amount too small");

Recommended Mitigation Steps

It is recommended to use != 0 instead of > 0.