search

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

5 stars 0 forks source link

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

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#L268

Vulnerability details

Proof-of-Concept

The PuttyV2.fillOrder is a payable function that allows users to transfer native ETH when filling 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.fillOrder function. The following table summarises whether or not the four (4) key workflows within the PuttyV2.fillOrder function requires native ETH transfer:

Workflows Accept or Require Native ETH Transfer?
Long Call No
Short Call Yes (For transfering native ETH premium from taker)
Long Put Yes (For transferring native ETH strike from taker )
Short Put Yes (For transfering native ETH premium from taker)

Thus, it is possible that the users accidentially attached non-zero Ether value to the transaction when calling PuttyV2.fillorder function when executing the Long Call 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#L268

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {

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, the Long Call workflow does not expect any native ETH to be sent. Therefore, if msg.value > 0, it should reverts.

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
    // If baseAsset is not WETH, there shouldn't be any native ETH attached
    if (order.baseAsset != weth && msg.value > 0) {
        revert();
    }
    // If filling long call, 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