L-02 : In exercise(), the positionExpirations[] check should be inclusive of block.timestamp
L-03 : In fillOrder(), the order.expiration check should be inclusive of block.timestamp
L-04 : Possibility of losing control of PuttyV2 contract via renounceOwnership()
L-05 : Possibility of losing control of PuttyV2 contract via transferOwnership()
L-06 : Return value during transfer of premium to order.maker not checked when weth used
Details L-01
Title : pause and unpause functionality missing
The PuttyV2 contract will hold good amount of TVL (Weth, ERC20's and ERC721's balances).
Impact
In case of any eventuality, there may be a requirement to freeze some of the operations of the protocol.
Hence a pause and unpause mechanism will help during that period.
Recommended Mitigation Steps
Incorporate or import the relevant library to pause/unpause few external functions.
Details L-02
Title : In exercise(), the positionExpirations[] check should be inclusive of block.timestamp
In the function exercise(),
line#401 require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
The check for positionExpirations should ideally include the block.timestamp as an edge case.
The reason is that in withdraw() function, the block.timestamp is exclusive
line#481 require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
Recommended Mitigation Steps
line#401 require(block.timestamp <= positionExpirations[uint256(orderHash)], "Position has expired");
Details L-03
Title : In fillOrder(), the order.expiration check should be inclusive of block.timestamp
In the function fillOrder(),
line#290 require(block.timestamp < order.expiration, "Order has expired");
The check for order.expiration should ideally include the block.timestamp as an edge case.
Recommended Mitigation Steps
line#290 require(block.timestamp <= order.expiration, "Order has expired");
Details L-04
Title : Possibility of losing control of PuttyV2 contract via renounceOwnership()
In PuttyV2.sol the PuttyV2 contract inherits Ownable, which has the function renounceOwnership()
If by mistake or maliciously the renounceOwnership() is called, there is a possibility of losing control on the PuttyV2 contract.
Recommended Mitigation Steps
Override this renounceOwnership() function in PuttyV2.sol by making it a no-op.
Details L-05
Title : Possibility of losing control of PuttyV2 contract via transferOwnership()
In PuttyV2.sol the PuttyV2 contract inherits Ownable, which has the function transferOwnership() which only checks for null address.
If by mistake a wrong address is used in transferOwnership(), then there is a possibility of losing control on the PuttyV2 contract.
Recommended Mitigation Steps
Best practice is to use two step process to transfer ownership
1st Step : setNewOwner << setting/proposing the address of the new Owner
2nd Step : acceptOwnership << the new Owner will execute this function to complete the ownership transfer
Details L-06
Title : Return value during transfer of premium to order.maker not checked when weth used
In fillOrder() , the return value during transfer of premium to order.maker is not checked when weth is used.
Summary of Findings for Low / Non-Critical issues
Details L-01
Title : pause and unpause functionality missing
The PuttyV2 contract will hold good amount of TVL (Weth, ERC20's and ERC721's balances).
Impact
In case of any eventuality, there may be a requirement to freeze some of the operations of the protocol. Hence a pause and unpause mechanism will help during that period.
Recommended Mitigation Steps
Incorporate or import the relevant library to pause/unpause few external functions.
Details L-02
Title : In exercise(), the positionExpirations[] check should be inclusive of block.timestamp
In the function exercise(),
The reason is that in withdraw() function, the block.timestamp is exclusive
Recommended Mitigation Steps
Details L-03
Title : In fillOrder(), the order.expiration check should be inclusive of block.timestamp
In the function fillOrder(),
Recommended Mitigation Steps
Details L-04
Title : Possibility of losing control of PuttyV2 contract via renounceOwnership()
In PuttyV2.sol the PuttyV2 contract inherits Ownable, which has the function renounceOwnership() If by mistake or maliciously the renounceOwnership() is called, there is a possibility of losing control on the PuttyV2 contract.
Recommended Mitigation Steps
Override this renounceOwnership() function in PuttyV2.sol by making it a no-op.
Details L-05
Title : Possibility of losing control of PuttyV2 contract via transferOwnership()
In PuttyV2.sol the PuttyV2 contract inherits Ownable, which has the function transferOwnership() which only checks for null address. If by mistake a wrong address is used in transferOwnership(), then there is a possibility of losing control on the PuttyV2 contract.
Recommended Mitigation Steps
Best practice is to use two step process to transfer ownership 1st Step : setNewOwner << setting/proposing the address of the new Owner 2nd Step : acceptOwnership << the new Owner will execute this function to complete the ownership transfer
Details L-06
Title : Return value during transfer of premium to order.maker not checked when weth used
In fillOrder() , the return value during transfer of premium to order.maker is not checked when weth is used.
In case of any failure in this line, the fillOrder will still succeed.
Recommended Mitigation Steps
Store the return value and check if success.