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

5 stars 0 forks source link

QA Report #412

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
  1. Block.timestamp would do missbehavior

block.timestamp is a value that keeps growing. It's the timestamp of the EVM, so it's increasing by the seconds. checking positionExpirations[uint256(orderHash)] will throw error that "Position has expired".

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

        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");

but in this case, checking block.timestamp > positionExpirations[longPositionId] || isExercised would throw and error that should be "Must not be exercised or expired" either.

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

        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

it should be check if less than "<" positionExpirations[longPositionId] || isExercised

  1. constants can be used than using numbers

1.) File : PuttyV2.sol Line.241

        require(_fee < 30, "fee must be less than 3%");        

2.) File : PuttyV2.sol Line.287

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

3.) File : PuttyV2.sol Line.499

                feeAmount = (order.strike * fee) / 1000;
  1. Reorder ERC20Asset and ERC721Asset typehash for better

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

Since usually ERC20 was the first being put in order it can be changed into ERC20Asset first then ERC721Asset typehash for better code readibility and use.

  1. Comment & Reason string consistency

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

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

Since it was for check if order.duration less than 10_000 days, and the comment said that check duration is valid. Reason string can be changed if it wasn't intended to not valid Duration.