search

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

5 stars 0 forks source link

When ETH is sent with `fillOrder` and `exercise` functions, the ETH can be lost #372

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 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L389

Vulnerability details

Impact

HIGH - assets can be lost

When an user is calling fillOrder or exercise functions, if the user sent some ETH accidentally, the ETH can be lost.

Proof of Concept

In the proof of concept, babe creates a short put order. The baseAsset for this order is set to some erc20 other than WETH. Bob calls fillOrder but sends some ETH accidentally. All transaction passes without failing and Bob lost the ETH he sent in.

This can happen in both fillOrder and exercise. Below is a table for the possible cases (, but not tested).

Order type function call condition
long put fillOrder non weth baseAsset
long call fillOrder
short put fillOrder non weth baseAsset
short call fillOrder non weth baseAsset
long put exercise
long call exercise non weth baseAsset

Tools Used

foundry

Recommended Mitigation Steps

The check for msg.value should be added for cases, where user is not supposed to send any ETH in.

rotcivegaf commented 2 years ago

Move to 2 (Med Risk) label Duplicate of #226

GalloDaSballo commented 2 years ago

Why would the caller be so generous to send eth when they don't need to?

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