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

5 stars 0 forks source link

Native funds sent over with an order are lost when not used #388

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#L322-L364

Vulnerability details

When there are native funds attached to an order that doesn't use them, for example having eth == order.baseAsset instead of weth as an operational mistake, these funds will end up being frozen within the contract. I.e. now there are no checks for such cases and no mechanics to retrieve the corresponding funds.

Setting severity to medium as that user funds freeze conditional on a range of operational mistakes that have significant cumulative probability.

Proof of Concept

Now there are no checks for the case when msg.value isn't used:

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

            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

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

        if (order.isLong) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
        }

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

            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
            }

Recommended Mitigation Steps

As there are several places when msg.value can be utilized, consider introducing a flag to track whether it was used, for example:

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


    bool nativeValueUsed;

    ...

    if (weth == order.baseAsset && msg.value > 0) {
        nativeValueUsed = true;
        // check enough ETH was sent to cover the premium

    ...

    require(nativeValueUsed || msg.value == 0, "Non-zero ETH amount"); 
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