search

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

5 stars 0 forks source link

Weak `msg.value` checks in `fillOrder` can make users lose ETH #360

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

Vulnerability details

Impact

Users can accidentally lose ETH when filling an order.

Proof of Concept

I believe that there is an logic error in your contract.

In your design when baseAsset == WETH users have two option.

Send ETH or send WETH to the contract (for example the case when Bob fills Alice long put order).

However when baseAsset != WETHit is possible for users to send native ETH. If the transactions succeed the ETH of filler will be locked in the contract forever.

Recommended Mitigation Steps

Add 

if(baseAsset!= WETH)

    require(msg.value == 0,"");

or similar to prevent users from losing ETH.

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