User may lose funds if ```msg.value > 0``` and ```baseAsset != weth```

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L327-L339 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L351-L361 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L427-L437

Vulnerability details

Impact

When filling an order, a user may have a non-zero msg.value when the order.baseAsset is not weth. This could lead to the user losing their funds as the msg.value is transferred directly to the PuttyV2.sol contract and not used.

Since the user would need to approve the PuttyV2.sol contract to transfer baseAsset, the possibility for the user to lose ETH is not high but should not be possible.

Proof of Concept

Functions affected:

• fillOrder

• exercise

Example for loss of premium in fillOrder:

1. Call fillOrder for a short call or a short put with msg.value == order.premium and order.baseAsset != weth.

2. Since the baseAsset != weth and the order is short, the ETH gained by the contract will not be converted to weth.

3. The baseAsset premium is sent from the caller of fillOrder to the order maker and ETH premium sent from the caller stays in the contract.

Similarly, a user could lose ETH strike amount if calling fillOrder for a long put with baseAsset != weth.

In exercise, the caller could lose ETH strike amount if the order is a call and baseAsset != weth.

_

Test code added to FillOrder.t.sol :

This test code creates a short put order with order.baseAsset == link and supplies the calling contract with link and ETH to the amount of order.premium. The fillOrder function is then called with a msg.value == order.premium. The test then checks that the calling contract spends both ETH and link equal to the premium. It also checks that the PuttyV2.sol contract gains ETH equal to the premium amount.

function testLossOfETH() public {
        PuttyV2.Order memory order = defaultOrder();
        order.isCall = false;
        order.isLong = false;
        order.maker = babe;
        order.floorTokens = new address[](0);
        order.baseAsset = address(link);
        order.duration = bound(order.duration, 0, 10_000 days);
        order.premium = bound(order.premium, 0, type(uint256).max - order.strike);
        order.strike = bound(order.strike, 0, type(uint256).max - order.premium);
        order.expiration = block.timestamp + 1 days;

        whitelist.push(address(this));
        order.whitelist = whitelist;
        bytes memory signature = signOrder(babePrivateKey, order);
        bytes32 orderHash = p.hashOrder(order);

        deal(address(link), address(this), order.premium);
        link.approve(address(p), order.premium);

        vm.startPrank(babe);
        deal(address(link), babe, order.strike);
        link.approve(address(p), order.strike);
        vm.stopPrank();

        // Add ETH to this contract
        deal(address(this), order.premium);

        // Balances before
        uint256 callerETHBalanceBefore = address(this).balance;
        uint256 callerLinkBalanceBefore = link.balanceOf(address(this));
        uint256 puttyETHBalanceBefore = address(p).balance;
        uint256 makerLinkBalanceBefore = link.balanceOf(address(babe));

        // act
        uint256 positionId = p.fillOrder{value: order.premium}(order, signature, floorAssetTokenIds);

        // Balances after
        uint256 callerETHBalanceAfter = address(this).balance;
        uint256 callerLinkBalanceAfter = link.balanceOf(address(this));
        uint256 puttyETHBalanceAfter = address(p).balance;
        uint256 makerLinkBalanceAfter = link.balanceOf(address(babe));

        assertEq(callerETHBalanceBefore - callerETHBalanceAfter, order.premium);         // Shows loss of ETH from caller.
        assertEq(puttyETHBalanceAfter - puttyETHBalanceBefore, order.premium);           // Shows PuttyV2.sol gains the ETH premium
        assertEq(callerLinkBalanceBefore - callerLinkBalanceAfter, order.premium);       // Shows this contract (caller) also loses link

Recommended Mitigation Steps

Revert if baseAsset != weth and msg.value > 0 in the fillOrder and exercise functions.

[rotcivegaf]

Duplicate of #226

[outdoteth]

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