Putty contest - rfa's results

Lines of code

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

Vulnerability details

Impact

If User send eth but he doesn't set another baseAssets instead of weth, the fund will just lost with no revert

Proof of Concept

The condition expect that user will using weth address as order.baseAsset when he trying to send ether to the contract. If he send eth with any token address (not weth address), the eth will just lost. In current implementation, those condition wont't executed and will executed at L338 (sending order.baseAsset to recipient).

Tools Used

Manual review

Recommended Mitigation Steps

Change the checks to:

            if (msg.value > 0) {
		//validate weth address if msg value != 0
		require(weth == order.baseAsset, "Invalid base asset");
                // check enough ETH was sent to cover the premium
                require(msg.value == order.premium, "Incorrect ETH amount sent");

I think it's necessary to make the contracts saver by doing this way

GAS

[G-1] Using calldata as a pointer for read only argument

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L271 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L657 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L669

Using calldata to store read only array variable can save gas

[G-2] Using != operator

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L293 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L598-L599

Using != rather than > to validate value is not 0 for uint is more effective

[G-3] Using unchecked and prefix increment for i inside for()

Occurrences: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742

Using unchecked and prefix increment is way more effective than the current implementation

RECOMMENDED MITIGATION STEP

        for (uint256 i = 0; i < orders.length;) {
            positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]);
	    unchecked{++i;}
        }

[G-4] Using else statement can save gas

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L509

Using else statement can avoid MLOADing and validating order.isCall and order.isLong. All the rest of conditions has checked before (L495). There are no more condition will occur at L509

RECOMMENDED MITIGATION CHECK

        if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
            // send the fee to the admin/DAO if fee is greater than 0%
            uint256 feeAmount = 0;
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

            return;
        }

        // transfer assets from putty to owner if put is exercised or call is expired
        else{ //@audit: Use else statement here
            _transferERC20sOut(order.erc20Assets);
            _transferERC721sOut(order.erc721Assets);

            // for call options the floor token ids are saved in the long position in fillOrder(),
            // and for put options the floor tokens ids are saved in the short position in exercise()
            uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash);
            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]);

            return;
        }