Putty contest - c3phas's results

FINDINGS

No need to initialize variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File: PuttyV2.sol line 497

            uint256 feeAmount = 0;

Other instances to modify File: PuttyV2.sol line 556

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

For the loops my suggestion would be to modify them as follows

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

Using unchecked blocks to save gas - Increments in for loop can be unchecked

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File: PuttyV2.sol line 556

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

The above should be modified to:

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

Other Instances to modify File: PuttyV2.sol line 594

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

File: PuttyV2.sol line 611

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

File: PuttyV2.sol line 627

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

File: PuttyV2.sol line 637

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

File: PuttyV2.sol line 647

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

File: PuttyV2.sol line 658

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

File: PuttyV2.sol line 670

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

File: PuttyV2.sol line 728

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

File: PuttyV2.sol line 742

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

see resource

Cache the length of arrays in loops -( saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: PuttyV2.sol line 556

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

The above should be modified to

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

Other instances to modify:

File: PuttyV2.sol line 594

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

File: PuttyV2.sol line 611

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

File: PuttyV2.sol line 627

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

File: PuttyV2.sol line 637

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

File: PuttyV2.sol line 647

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

File: PuttyV2.sol line 658

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

File: PuttyV2.sol line 670

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

File: PuttyV2.sol line 728

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

File: PuttyV2.sol line 742

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

++i costs less gas compared to i++ or i += 1 (saves ~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include: File: PuttyV2.sol line 556

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

File: PuttyV2.sol line 594

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

File: PuttyV2.sol line 611

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

File: PuttyV2.sol line 627

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

File: PuttyV2.sol line 637

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

File: PuttyV2.sol line 647

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

File: PuttyV2.sol line 658

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

File: PuttyV2.sol line 670

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

File: PuttyV2.sol line 728

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

File: PuttyV2.sol line 742

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

Use custom errors rather than revert()/require() strings to save deployment gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

see Source

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

File: PuttyV2.sol line 214

        require(_weth != address(0), "Unset weth address");

File: PuttyV2.sol line 241

        require(_fee < 30, "fee must be less than 3%");

File: PuttyV2.sol line 278 File: PuttyV2.sol line 281 File: PuttyV2.sol line 284 File: PuttyV2.sol line 287 File: PuttyV2.sol line 290 File: PuttyV2.sol line 293 File: PuttyV2.sol line 297-298 File: PuttyV2.sol line 329 File: PuttyV2.sol line 353 File: PuttyV2.sol line 395 File: PuttyV2.sol line 398 File: PuttyV2.sol line 401 File: PuttyV2.sol line 405&406 File: PuttyV2.sol line 429 File: PuttyV2.sol line 470 File: PuttyV2.sol line 475 File: PuttyV2.sol line 481 File: PuttyV2.sol line 527 File: PuttyV2.sol line 551-552 File: PuttyV2.sol line 598-599 File: PuttyV2.sol line 765

File: PuttyV2Nft.sol line 12-13 File: PuttyV2Nft.sol line 26-28 File: PuttyV2Nft.sol line 41

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

consequences:

• Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

• Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

See: ethereum/solidity#9232

File: PuttyV2.sol line 89

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

File: PuttyV2.sol line 95

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

File: PuttyV2.sol line 101

Constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended.

File: PuttyV2.sol line 499

                feeAmount = (order.strike * fee) / 1000;