PartyDAO contest - 0x1f8b's results

• Low
  • 1. Unsafe math
  • 2. Integer overflow by unsafe casting
  • 3. Mixing and Outdated compiler
  • 4. Lack of checks
  • 5. Same length of arrays not checked
• Non critical
  • 6. Use abstract for base contracts
  • 7. Open TODO

Low

1. Unsafe math

A user supplied argument can be passed as zero to multiplication operation.

Reference:

• https://slowmist.medium.com/analysis-of-the-treasuredao-zero-fee-exploit-73791f4b9c14

Affected source code:

• TokenDistributor.sol:259-260

2. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

    checkpoint.votes = uint192(_newTotalVotes);

Affected source code:

• AuctionCrowdfund.sol:118
• BuyCrowdfundBase.sol:73
• TokenDistributor.sol:259
• Globals.sol:36
• Globals.sol:40
• Globals.sol:44
• PartyGovernance.sol:190
• PartyGovernance.sol:539
• PartyGovernance.sol:605
• PartyGovernance.sol:684
• PartyGovernance.sol:687
• PartyGovernance.sol:695
• PartyGovernance.sol:755
• PartyGovernance.sol:756
• PartyGovernance.sol:762
• PartyGovernance.sol:903
• PartyGovernance.sol:940
• PartyGovernance.sol:963
• PartyGovernance.sol:1033
• PartyGovernance.sol:1036
• PartyGovernance.sol:1078
• PartyGovernance.sol:1079
• ListOnOpenseaProposal.sol:152
• ListOnOpenseaProposal.sol:154
• ListOnOpenseaProposal.sol:202
• ListOnOpenseaProposal.sol:203
• ListOnOpenseaProposal.sol:210
• ListOnZoraProposal.sol:94
• ListOnZoraProposal.sol:95
• ListOnZoraProposal.sol:102
• ListOnZoraProposal.sol:158
• ListOnZoraProposal.sol:159
• ListOnZoraProposal.sol:188
• ProposalExecutionEngine.sol:221
• ProposalExecutionEngine.sol:247

3. Mixing and Outdated compiler

The pragma version used are:

pragma solidity ^0.8;
pragma solidity >=0.8.0;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

4. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• Globals.sol:24
• Globals.sol:28
• FractionalizeProposal.sol:35
• PartyGovernance.sol:267
• TokenDistributor.sol:94
• ListOnOpenseaProposal.sol:114-116
• ListOnZoraProposal.sol:71-72
• CrowdfundFactory.sol:26
• CrowdfundNFT.sol:35
• Crowdfund.sol:119

5. Same length of arrays not checked

It iterates and checks over two arrays, assuming that both have the same length, which can cause the method not to detect what it has to detect, as well as a denial of service when called incorrectly.

Affected source code:

• LibProposal.sol:25-26

---

Non critical

6. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• EIP165.sol:4
• Proxy.sol:8
• ReadOnlyDelegateCall.sol:14
• ERC1155Receiver.sol:7
• ERC721Receiver.sol:11
• Globals.sol:7
• ProposalStorage.sol:11

7. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

TODO: make this human readable

• DummyERC721Renderer.sol:8

• Gas
  • 1. Use require instead of assert
  • 2. Don't use the length of an array for loops condition
  • 3. There's no need to set default values for variables
    • Total gas saved: 8 * 15 = 120
  • 4. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 18 = 234
  • 5. Shift right instead of dividing by 2
    • Total gas saved: 172 * 1 = 172
  • 6. Remove empty blocks
  • 7. Use calldata instead of memory
  • 8. ++i costs less gas compared to i++ or i += 1
  • 9. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 28 = 252
  • 10. Avoid unused returns
  • 11. constants expressions are expressions, not constants
  • 12. Gas saving using immutable
  • 13. Use the unchecked keyword
  • 14. Optimize CollectionBuyCrowdfund.onlyHost

Gas

1. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• CrowdfundNFT.sol:166
• TokenDistributor.sol:385
• TokenDistributor.sol:411
• PartyGovernance.sol:504
• PartyGovernanceNFT.sol:179
• FractionalizeProposal.sol:67
• ListOnOpenseaProposal.sol:221
• ListOnOpenseaProposal.sol:302
• ListOnZoraProposal.sol:120
• ProposalExecutionEngine.sol:142
• ReadOnlyDelegateCall.sol:30

2. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• CollectionBuyCrowdfund.sol:62
• Crowdfund.sol:180
• Crowdfund.sol:300
• TokenDistributor.sol:230
• TokenDistributor.sol:239
• ArbitraryCallsProposal.sol:52
• ArbitraryCallsProposal.sol:61
• ArbitraryCallsProposal.sol:78
• LibProposal.sol:14
• LibProposal.sol:32
• ListOnOpenseaProposal.sol:291
• ERC1155.sol:80
• ERC1155.sol:118

3. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• CollectionBuyCrowdfund.sol:62
• Crowdfund.sol:180
• Crowdfund.sol:300
• TokenDistributor.sol:230
• TokenDistributor.sol:239
• ArbitraryCallsProposal.sol:52
• ArbitraryCallsProposal.sol:61
• ArbitraryCallsProposal.sol:78
• LibProposal.sol:14
• LibProposal.sol:32
• ListOnOpenseaProposal.sol:291
• ERC1155.sol:80
• ERC1155.sol:118
• PartyGovernance.sol:306
• PartyGovernance.sol:432

Total gas saved: 8 * 15 = 120

4. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• Crowdfund.sol:411
• TokenDistributor.sol:381
• ERC1155.sol:51
• ERC1155.sol:52
• ERC1155.sol:84
• ERC1155.sol:85
• ERC1155.sol:145
• ERC1155.sol:169
• ERC1155.sol:199
• ERC1155.sol:216
• ERC20.sol:76
• ERC20.sol:81
• ERC20.sol:98
• ERC20.sol:103
• ERC20.sol:183
• ERC20.sol:188
• ERC20.sol:195
• ERC20.sol:200

Total gas saved: 13 * 18 = 234

5. Shift right instead of dividing by 2

Shifting one to the right will calculate a division by two.

he SHR opcode only requires 3 gas, compared to the DIV opcode's consumption of 5. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testDiv(uint a) public returns (uint) { return a / 2; }
}

contract TesterB {
function testShift(uint a) public returns (uint) { return a >> 1; }
}

Gas saving executing: 172 per entry

TesterA.testDiv:    21965
TesterB.testShift:  21793   

Affected source code:

• PartyGovernance.sol:434

Total gas saved: 172 * 1 = 172

6. Remove empty blocks

An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.

Sample code:

pragma solidity >=0.4.0 <0.7.0;

abstract contract BaseContract {
    function totalSupply() public virtual returns (uint256);
}

Reference:

• https://docs.soliditylang.org/en/v0.6.0/contracts.html?highlight=abstract#abstract-contracts

Affected source code:

• AuctionCrowdfund.sol:104
• AuctionCrowdfund.sol:144
• BuyCrowdfund.sol:62
• BuyCrowdfundBase.sol:67
• CollectionBuyCrowdfund.sol:77
• CrowdfundNFT.sol:49-52
• CrowdfundNFT.sol:56-59
• CrowdfundNFT.sol:63-66
• CrowdfundNFT.sol:70-73
• CrowdfundNFT.sol:77-80
• Party.sol:28
• Party.sol:47
• ProposalExecutionEngine.sol:99-103

7. Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

-   function initialize(AuctionCrowdfundOptions memory opts)
+   function initialize(AuctionCrowdfundOptions calldata opts)
        external
        payable
        onlyConstructor
    {
    ...
    }

Affected source code:

• AuctionCrowdfund.sol:110-141
• AuctionCrowdfund.sol:196-259
• BuyCrowdfund.sol:68-88
• CollectionBuyCrowdfund.sol:83-102
• Party.sol:33-45
• ProposalExecutionEngine.sol:116-180

8. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, 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 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• CollectionBuyCrowdfund.sol:62
• ERC20.sol:142
• ERC721.sol:98
• ERC721.sol:100
• ERC721.sol:163
• ERC721.sol:178

9. Use Custom Errors instead of Revert Strings to save 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)

Source Custom Errors in Solidity:

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).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• ProposalExecutionEngine.sol:246
• ProposalExecutionEngine.sol:247
• ReadOnlyDelegateCall.sol:20
• ERC1155.sol:49
• ERC1155.sol:56-62
• ERC1155.sol:72
• ERC1155.sol:74
• ERC1155.sol:96-102
• ERC1155.sol:111
• ERC1155.sol:149-155
• ERC1155.sol:166
• ERC1155.sol:180-186
• ERC1155.sol:196
• ERC20.sol:124
• ERC20.sol:153
• ERC721.sol:35
• ERC721.sol:39
• ERC721.sol:68
• ERC721.sol:86
• ERC721.sol:88
• ERC721.sol:90-93
• ERC721.sol:117-122
• ERC721.sol:133-138
• ERC721.sol:157
• ERC721.sol:159
• ERC721.sol:174
• ERC721.sol:195-200
• ERC721.sol:210-215

Total gas saved: 9 * 28 = 252

10. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• ERC20.sol:67-73
• ERC20.sol:75-87
• ERC20.sol:89-109

11. constants expressions are expressions, not constants

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.

Reference:

• https://github.com/ethereum/solidity/issues/9232

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).

Affected source code:

• ProposalStorage.sol:19
• ProposalExecutionEngine.sol:80

12. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• ERC721.sol:20
• ERC721.sol:22
• ERC20.sol:20
• ERC20.sol:22

13. Use the unchecked keyword

When an underflow or overflow cannot occur, one might conserve gas by using the unchecked keyword to prevent unnecessary arithmetic underflow/overflow tests.

    function safeTransferFrom(
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes calldata data
    ) public virtual {
        require(msg.sender == from || isApprovedForAll[from][msg.sender], "NOT_AUTHORIZED");

        balanceOf[from][id] -= amount;
-       balanceOf[to][id] += amount;
+       unchecked {
+           balanceOf[to][id] += amount;
+       }   

        emit TransferSingle(msg.sender, from, to, id, amount);

        require(
            to.code.length == 0
                ? to != address(0)
                : ERC1155TokenReceiverBase(to).onERC1155Received(msg.sender, from, id, amount, data) ==
                    ERC1155TokenReceiverBase.onERC1155Received.selector,
            "UNSAFE_RECIPIENT"
        );
    }

Affected source code:

• ERC1155.sol:52
• ERC1155.sol:85

14. Optimize CollectionBuyCrowdfund.onlyHost

It's possible to remove the variable isHost like this:

    modifier onlyHost(address[] memory hosts) {
-       bool isHost;
        for (uint256 i; i < hosts.length; i++) {
            if (hosts[i] == msg.sender) {
-               isHost = true;
+               _;
+               return;
-               break;
            }
        }

+       revert OnlyPartyHostError();
-       if (!isHost) {
-           revert OnlyPartyHostError();
-       }
-       _;
    }

Affected source code:

• CollectionBuyCrowdfund.sol:60-74