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

5 stars 0 forks source link

Gas Optimizations #270

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[G-01] Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

./PuttyV2.sol:297:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:405:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:353:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:429:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2Nft.sol:12:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:27:        require(to != address(0), "INVALID_RECIPIENT");

Mitigation

I recommend making duplicate require statement into modifier or a function.

[G-02] Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./PuttyV2.sol:210:        string memory _baseURI,
./PuttyV2.sol:228:    function setBaseURI(string memory _baseURI) public payable onlyOwner {
./PuttyV2.sol:269:        Order memory order,
./PuttyV2.sol:389:    function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
./PuttyV2.sol:466:    function withdraw(Order memory order) public {
./PuttyV2.sol:526:    function cancel(Order memory order) public {
./PuttyV2.sol:547:        Order[] memory orders,
./PuttyV2.sol:574:        Order memory order,
./PuttyV2.sol:576:        Order memory originalOrder
./PuttyV2.sol:593:    function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
./PuttyV2.sol:610:    function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
./PuttyV2.sol:636:    function _transferERC20sOut(ERC20Asset[] memory assets) internal {
./PuttyV2.sol:646:    function _transferERC721sOut(ERC721Asset[] memory assets) internal {
./PuttyV2.sol:657:    function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
./PuttyV2.sol:669:    function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) {
./PuttyV2.sol:683:    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
./PuttyV2.sol:699:    function hashOrder(Order memory order) public view returns (bytes32 orderHash) {
./PuttyV2.sol:727:    function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
./PuttyV2.sol:741:    function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {

Mitigation

Change memory to calldata

[G-03] Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

  1. PuttyV2.sol

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

[G-04] Both named returns and return statement are used

Issue

In some function return statement are used even though named returns is set. This is redundant because return statement is not needed when using named returns and named returns is not needed when using return statement. Removing unused named returns variable in below code can save gas and improve code readability.

PoC

  1. PuttyV2.sol

Remove returns variable "positionId" https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L272 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L363 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L370 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L378

Mitigation

Remove unused named returns variable as mentioned in above PoC.

[G-05] Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this "constant". link for more details: https://github.com/ethereum/solidity/issues/9232

PoC

89:    bytes32 public constant ERC721ASSET_TYPE_HASH =
90:        keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

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

95:    bytes32 public constant ERC20ASSET_TYPE_HASH =
96:        keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

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

101:    bytes32 public constant ORDER_TYPE_HASH =
102:        keccak256(
103:            abi.encodePacked(
104:                "Order(",
105:                "address maker,",
106:                "bool isCall,",
107:                "bool isLong,",
108:                "address baseAsset,",
109:                "uint256 strike,",
110:                "uint256 premium,",
111:                "uint256 duration,",
112:                "uint256 expiration,",
113:                "uint256 nonce,",
114:                "address[] whitelist,",
115:                "address[] floorTokens,",
116:                "ERC20Asset[] erc20Assets,",
117:                "ERC721Asset[] erc721Assets",
118:                ")",
119:                "ERC20Asset(address token,uint256 tokenAmount)",
120:                "ERC721Asset(address token,uint256 tokenId)"
121:            )
122:        );

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

Mitigation

Change the variable from constant to immutable.

[G-06] Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

./PuttyV2.sol:497:            uint256 feeAmount = 0;
./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

I suggest removing default value initialization. For example,

[G-07] Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Store array's length as a variable before looping it. For example, I suggest changing it to

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

[G-08] ++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Change it to postfix increments/decrements. For example,

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

[G-09] != 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in "require" statement.

PoC

./PuttyV2.sol:293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
./PuttyV2.sol:598:            require(token.code.length > 0, "ERC20: Token is not contract");
./PuttyV2.sol:599:            require(tokenAmount > 0, "ERC20: Amount too small");

Mitigation

I suggest changing > 0 to != 0

require(order.baseAsset.code.length != 0, "baseAsset is not contract");

[G-10] Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

./PuttyV2.sol:214:        require(_weth != address(0), "Unset weth address");
./PuttyV2.sol:241:        require(_fee < 30, "fee must be less than 3%");
./PuttyV2.sol:278:        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");
./PuttyV2.sol:281:        require(!cancelledOrders[orderHash], "Order has been cancelled");
./PuttyV2.sol:284:        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
./PuttyV2.sol:287:        require(order.duration < 10_000 days, "Duration too long");
./PuttyV2.sol:290:        require(block.timestamp < order.expiration, "Order has expired");
./PuttyV2.sol:293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
./PuttyV2.sol:297:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:298:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");
./PuttyV2.sol:329:                require(msg.value == order.premium, "Incorrect ETH amount sent");
./PuttyV2.sol:353:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:398:        require(order.isLong, "Can only exercise long positions");
./PuttyV2.sol:401:        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
./PuttyV2.sol:405:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:406:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
./PuttyV2.sol:429:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:470:        require(!order.isLong, "Must be short position");
./PuttyV2.sol:475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:481:        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
./PuttyV2.sol:527:        require(msg.sender == order.maker, "Not your order");
./PuttyV2.sol:551:        require(orders.length == signatures.length, "Length mismatch in input");
./PuttyV2.sol:552:        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");
./PuttyV2.sol:598:            require(token.code.length > 0, "ERC20: Token is not contract");
./PuttyV2.sol:599:            require(tokenAmount > 0, "ERC20: Amount too small");
./PuttyV2.sol:765:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");
./PuttyV2Nft.sol:12:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:13:        require(_ownerOf[id] == address(0), "ALREADY_MINTED");
./PuttyV2Nft.sol:26:        require(from == _ownerOf[id], "WRONG_FROM");
./PuttyV2Nft.sol:27:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:28-31:        require(msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id], "NOT_AUTHORIZED");
./PuttyV2Nft.sol:41:        require(owner != address(0), "ZERO_ADDRESS");

Mitigation

I suggest implementing custom errors to save gas.