search

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

5 stars 0 forks source link

Gas Optimizations #280

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Executive Summary

The codebase is already pretty well thought out, by extending the idea of using ERC721 IDs as identifiers, we can save massive amount of gas for day to day operations, we can also apply a few basic logic transformations as well as basic gas saving tips to reduce the total gas for the average operation by over 20k gas

The first few refactoring will offer strong gas savings with minimal work (20k+ gas), the remaining findings are minor and I expect most other wardens to also be suggesting them

Massive Gas Savings in exercise by removing exercisedPositions - Around 20k gas on every exercise (17k to 23k from foundry tests)

It seems like exercisedPositions is used exclusively for withdraw, however through the cleverly designed "send to 0xdead" system, we can replace the exercisedPositions function with the following

    function exercisedPositions(uint256 id) external view returns(bool) {
        return ownerOf(id) == address(0xdead);
    }

In fact, a position was exercised if it was transfered to 0xdead, as such there's no need to store a mapping in storage.

We can delete every mention of exercisedPositions as well as the storage mapping

This single change will reduce almost 20k gas from exercise

To make withdraw work we can write the following

        // Inlined is even cheaper (8 gas for the JUMP)
        bool isExercised = ownerOf(uint256(longPositionId)) == address(0xdead);

And we can delete the exercisedPositions mapping from the contract

You can use the function above so the test still passes, in case you need a way to verify if a position was exercised, otherwise you can just use the isExercised line and delete very other mention of exercisedPositions

Gas Math

BEFORE exercise ┆ 5759 ┆ 55920 ┆ 68002 ┆ 133534 ┆ 18
withdraw ┆ 3075 ┆ 27840 ┆ 24545 ┆ 71168 ┆ 10

AFTER exercise ┆ 5759 ┆ 42356 ┆ 45806 ┆ 115777 ┆ 18 withdraw ┆ 3075 ┆ 26968 ┆ 23807 ┆ 70836 ┆ 10

Diff

155c155,157
<     mapping(uint256 => bool) public exercisedPositions;
---
>     function exercisedPositions(uint256 id) external view returns(bool) {
>         return ownerOf(id) == address(0xdead);
>     }
415,417d416
<         // mark the position as exercised
<         exercisedPositions[uint256(orderHash)] = true;
< 
478c477
<         bool isExercised = exercisedPositions[longPositionId];
---
>         bool isExercised = ownerOf(longPositionId) == address(0xdead);

Avoid a SLOAD optimistically - 2.1k gas saved

Because you already computed isExercised, you can save an SLOAD (2.1k gas) if you check for isExercised first.

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

        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

You could also refactor to the reverse check (if you expect the majority of positions to expire worthless), either way the short circuit can be used to save a SLOAD, whicever priority you give it

Change to

        require(isExercised || block.timestamp > positionExpirations[longPositionId], "Must be exercised or expired");

To save 2.1k gas if the option was exercised

Double SLOAD - 94 gas saved

Everytime a Storage Variable is loaded twice, (SLOAD), it would be cheaper to use a supporting memory variable The cost of a Second SLOAD is 100 gas, the cost of an MSTORE is 3 and and MLOAD another 3 gas.

Using a supporting variable will save 94 gas on the second read, and 97 gas for each subsquent MSTORE

In the case of fee this can save 94 gas

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

            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;

Storage Pointer to floorTokenIds will save gas (avoid copying whole storage to memory) - Between 400 and 2k gas saved on Withdraw and Exercise - 400 / 2k * 2 gas saved

If you pass a storage pointer as argument to a memory typed function like _transferFloorsOut

    function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {

You are copying the storage values to memory and then reading them and looping through that copy, this incurs an overhead and is equivalent to looping over storage, copying into memory and then passing the memory variable.

This is one of the few cases where a storage declaration (passing the storage pointer) will save gas

Refactor to

    function _transferFloorsOut(address[] memory floorTokens, uint256[] storage floorTokenIds) internal {

Gas Math

Before

exercise ┆ 5759 ┆ 55920 ┆ 68002 ┆ 133534 ┆ 18 withdraw ┆ 3075 ┆ 27840 ┆ 24545 ┆ 71168 ┆ 10

 After

exercise ┆ 5759 ┆ 55176 ┆ 65795 ┆ 133534 ┆ 18
withdraw ┆ 3075 ┆ 27365 ┆ 24545 ┆ 71003 ┆ 10

Save gas by avoiding NOT - Saves 6 gas sometimes

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

        // check floor asset token ids length is 0 unless the position type is put
        !order.isCall
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

Because the logic is fully known, it would be cheaper to swap the if else to:

        order.isCall
            ? require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
            : require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")

As that would avoid the extra NOT

Check msg.value first - 1 gas per instance - 3 gas total

Reading msg.value costs 2 gas, while reading from memory costs 3, this will save 1 gas with no downside

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

if (weth == order.baseAsset && msg.value > 0) {

Change to

if (msg.value > 0 && weth == order.baseAsset) {

Common Gas Savings

Below are a bunch of basic gas saving tips you probably will receive in most competent submissions added below for the sake of completeness

Save 3 gas by not reading orders.length - 3 gas per instance - 27 gas saved

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

        require(orders.length == signatures.length, "Length mismatch in input");

orders.length is read multiple times, because of that, especially for the loop, you should cache it in a memory variable to save 3 gas per instance

Refactor to:

        ordersLength = orders.length;
        require(orders.length == signatures.length, "Length mismatch in input");

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

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

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

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

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

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

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

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

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

Avoid default assignment - 3 gas per instance - 27 gas saved

Declaring uint256 i = 0; means doing an MSTORE of the value 0 Instead you could just declare uint256 i to declare the variable without assigning it's default value, saving 3 gas per declaration

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

        for (uint256 i = 0; i < orders.length; i++) {

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

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

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

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

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

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

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

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

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

Cheaper For Loops - 25 to 80 gas per instance - 9 instances

You can get cheaper for loops (at least 25 gas, however can be up to 80 gas under certain conditions), by rewriting:

        for (uint256 i = 0; i < orders.length; /** NOTE: Removed i++ **/ ) {
                // Do the thing

                // Unchecked pre-increment is cheapest
                unchecked { ++i; }
        }

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

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

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

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

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

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

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

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

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

Simplify Value Comparison - Saves some bytecode and makes logic leaner

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

        if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {

Can be refactored with

        // transfer strike to owner if put is expired or call is exercised
        if (order.isCall == isExercised) {

As in both case they were both true or both false

Consequence of Smart Comparison Above - Saves 20 to 80 gas (avg is 60)

Because of the gas save above we can refactor the entire block to an if / else

        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
        if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) {
            _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;
        }

Becomes

        if (order.isCall == isExercised)) {
                // transfer assets from putty to owner if put is exercised or call is expired

                // 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);
        } else {
                // transfer assets from putty to owner if put is exercised or call is expired

                _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]);
        }

Which reduces bytecode size, and saves gas in most situations as you're avoiding up to 3 extra comparisons

outdoteth commented 2 years ago

cool idea with relying on 0xdead to see if an option is exercised or not. unfortunately this doesnt work because a user can intentionally "burn" their NFT to 0xdead which then messes up the logic in withdraw. This can be mitigated, but requires too many changes.

high quality report though.