Putty contest - hubble's results

Lines of code

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

Vulnerability details

Fees is calculated and sent only in the condition if put is expired or call is exercised No fees sent in either withdraw() or exercise() in condition if put is exercised or call is expired.

Impact

Loss of fees

Recommended Mitigation Steps

Calculate similar fees and send

Lines of code

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

Vulnerability details

The solidity version 0.8.13 has below two issues applicable to PuttyV2

1. Vulnerability related to ABI-encoding. ref : https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/ This vulnerability can be misused since the function hashOrder() and hashOppositeOrder() has applicable conditions. "...pass a nested array directly to another external function call or use abi.encode on it."

2. Vulnerability related to 'Optimizer Bug Regarding Memory Side Effects of Inline Assembly' ref : https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/ PuttyV2 inherits solidity contracts from openzeppelin and solmate, and both these uses inline assembly, and optimization is enabled while compiling.

Recommended Mitigation Steps

Use recent Solidity version 0.8.15 which has the fix for these issues

Summary of Findings for Low / Non-Critical issues

• L-01 : pause and unpause functionality missing
• 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.

line#336 IWETH(weth).transfer(order.maker, msg.value);

In case of any failure in this line, the fillOrder will still succeed.

Recommended Mitigation Steps

Store the return value and check if success.