search

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

5 stars 0 forks source link

QA Report #163

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

Few vulnerabilities were found. The main concerns are with:

Hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Two instances where abi.encodePacked is used with bytes arguments:

PuttyV2.sol

encoded = abi.encodePacked(encoded,keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount)))\ encoded = abi.encodePacked(encoded,keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId)))

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

_mint does not check if recipient can handle tokens

PROBLEM

_mint() defined in PuttyV2Nft.sol does not checks whether to can handle ERC721 tokens if it is not an EOA. The function is called in PuttyV2.fillOrder to mint the tokens representing the long and short position for the order. If either order.maker or the caller is a contract not handling ERC721 tokens, one of the minted tokens will be lost, which can result in either exercise or withdraw not being able to be called

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

scope: fillOrder

_mint(order.maker, uint256(orderHash))\ _mint(msg.sender, positionId)

TOOLS USED

Manual Analysis

MITIGATION

Add a check in _mint to ensure to implements an ERC721Receiver if it is not an EOA

Payable functions when using ERC20

PROBLEM

There should be a require(0 == msg.value) to ensure no Ether is being sent to PuttyV2.sol when the order.baseAsset used in an order is a ERC20 token. As there is no way to withdraw ETH from the contract, any ETH mistakenly sent when order.baseAsset is an ERC20 token will result in locked ETH in the contract.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

scope: fillOrder

scope: exercise

TOOLS USED

Manual Analysis

MITIGATION

Add require(0 == msg.value) in all blocks mentioned above.

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

require(_fee < 30, "fee must be less than 3%")\ require(order.duration < 10_000 days, "Duration too long")\ feeAmount = (order.strike * fee) / 1000

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events emitted early

PROBLEM

Events should be emitted at the end of a function call, to prevent emission if the function reverts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

All the following events are emitted before external calls are performed.

emit FilledOrder(orderHash, floorAssetTokenIds, order)\ emit ExercisedOrder(orderHash, floorAssetTokenIds, order)\ emit WithdrawOrder(orderHash, order)

TOOLS USED

Manual Analysis

MITIGATION

Emit events at the end of functions.

Events indexing

PROBLEM

Events should use indexed fields, to make them easier to filter in logs.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

event NewBaseURI(string baseURI)\ event NewFee(uint256 fee)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events.

Function order

PROBLEM

Functions should be ordered following the Soldiity conventions. This means ordering functions based on their visibility.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Several contracts have public functions defined after internal functions:

TOOLS USED

Manual Analysis

MITIGATION

Place the publicfunctions internal functions.

`

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

function exercise()\ function withdraw()\ function batchFillOrder()\ function acceptCounterOffer()\ function domainSeparatorV4()

TOOLS USED

Manual Analysis

MITIGATION

Declare this function as external instead of public

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

mapping(uint256 => uint256) public positionExpirations\ mapping(uint256 => bool) public exercisedPositions\ mapping(uint256 => uint256[]) public positionFloorAssetTokenIds

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, for the PuttyV2.sol mappings, the mitigation could be:


struct Position {
  uint256 expiration;
  bool exercised;
  uint256[] floorAssetTokenId;
}

And it would be used as a state variable:

mapping(uint256 => Position) public positions;

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e4) rather than decimal literals(10_000) or exponentiation(10**4)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

require(order.duration < 10_000 days, "Duration too long")\ feeAmount = (order.strike * fee) / 1000

TOOLS USED

Manual Analysis

MITIGATION

Replace the numbers aforementioned with their scientific notation. Consider also writing replacing two numbers with constant variables

Timelock on setters

PROBLEM

It is good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. Here it is merely a suggestion as setFee already has a very reasonable upper boundary.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

function setBaseURI\ function setFee

TOOLS USED

Manual Analysis

MITIGATION

Consider adding a timelock to these setters

TransferOwnership can be 2-steps

PROBLEM

The current ownership transfer process uses OpenZeppelin Ownable contract, where the current owner writes the next owner and the change is immediate. You can consider using 2-step process, where the new owner has to accept the ownership. preventing passing the ownership to an uncontrolled account.

SEVERITY

Non-Critical

TOOLS USED

Manual Analysis

Unused returns

PROBLEM

Adding a return statement when the function defines a named return variable is redundant.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PuttyV2.sol

positionId loops through the array of addresses and delete the address to remove when it finds it. The costs of calling this function may vary drastically depending on the location of the address in the array and how large the array is

TOOLS USED

Manual Analysis

MITIGATION

You don't need to explicitly return positionId.

outdoteth commented 2 years ago

_mint does not check if recipient can handle tokens

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

Payable functions when using ERC20

Duplicate: Native ETH can be lost if it’s not utilised in exercise and fillOrder: https://github.com/code-423n4/2022-06-putty-findings/issues/226

outdoteth commented 2 years ago

high quality report