search

code-423n4 / 2022-06-putty-findings

5 stars 0 forks source link

QA Report #306

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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:

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.

No. Title Risk Rating
3.1 Lack Of Reentrancy Guards On External Functions Low
3.2 Discontinuity in Exercise Period Low
3.3 Insufficient Input Validation Low
3.4 Order Cannot Be Filled Due To Unbounded Whitelist Within An Order Low
3.5 Order Cannot Be Filled Due To Unbounded floorTokens, ERC20Asset Or ERC721Asset Within An Order Low
3.6 Order Can Be Cancelled Even After Being Filled Low
3.7 No Check if onERC721Received Is Implemented Low
4.1 Omissions in events Non-Critical
4.2 Draft OpenZeppelin Dependencies Non-Critical
4.3 Insufficient Tests Non-Critical
4.4 Owner Can Renounce Ownership Non-Critical
4.5 Consider two-phase ownership transfer Non-Critical
4.6 Code Can Be Refactored To Be More Readable Non-Critical
4.7 Inconsistent use of named return variables Non-Critical
4.8 Unused imports Non-Critical
4.9 Incorrect functions visibility Non-Critical

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.

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:

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:

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:

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:

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

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

Recommendation

Implement NatSpec for all functions.

outdoteth commented 2 years ago

No Check if onERC721Received Is Implemented

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: https://github.com/code-423n4/2022-06-putty-findings/issues/327

outdoteth commented 2 years ago

high quality report

HickupHH3 commented 2 years ago

3.3 Insufficient Input Validation

Disagree, I remember seeing there was a use case for having taker == maker discussed in 1 of the issues somewhere.

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

Sort a duplicate of #290.

Agree that overall it is a very good report!