search

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

5 stars 0 forks source link

Payable Function `exercise()` Allows For Native ETH Transfer Causing Ether To Be Stuck #303

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Proof-of-Concept

The PuttyV2.exercise is a payable function that allows users to transfer native ETH when exercising an order. The native ETH is only applicable when the baseAsset is equal to WETH AND during the following scenarios:

In all other circumstances, native ETH transfer is not applicable within the PuttyV2.exercise function. The following table summarises whether or not the two (2) key workflows within the PuttyV2.exercise function requires native ETH transfer:

Workflows Accept or Require Native ETH Transfer?
Long Call Yes (For transferring native ETH strike from exerciser to Putty)
Long Put No

Thus, it is possible that the users accidentially attached non-zero Ether value to the transaction when calling PuttyV2.exercise function when executing the Long Put workflow. Thus, the user's Ether will be locked within the contract without the ability to retrieve it.

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

function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {

Impact

User's Ether locked in the contract.

Recommended Mitigation Steps

It is recommended to reverts if msg.value > 0 for workflows or scenarios that Native ETH is not expected or required.

When the order.baseAsset is not equal to WETH AND `msg.value > 0 `, it should reverts.

Additionally, when the user attempts to exercise a "Long Put" order, reverts if msg.value > 0 as exercising a "Long Put" does not expect the users to send any native ETH.

function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
    // If baseAsset is not WETH, there shouldn't be any native ETH attached
    if (order.baseAsset != weth && msg.value > 0) {
        revert();
    }
    // If excercising long put, there shouldn't be any native ETH attached
    if (order.isLong && !order.isCall && msg.value > 0) {
        revert();
    }
    ..SNIP..
rotcivegaf commented 2 years ago

A part duplicate of #226

outdoteth commented 2 years ago

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