Party DAO - lsaudit &#x27;s results

        // If the deployer passed in some ETH during deployment, credit them
        // for the initial contribution.
        uint96 initialContribution = msg.value.safeCastUint256ToUint96()

According to above comment, initialContribution should take into consideration not only msg.value, but also the ETH which might had been sent during the deployment process. However, the current implementation does not take that pre-existing ETH into consideration. This comment is very misleading to the user and creates a risk of fund-loss (thus it was categarized as Low). User may send ETH before calling _initialize(), hoping that, these ETH will be added to initialContribution.

Our recommendation is to either calculate pre-existing ETH tokens in initialContribution or remove that comment.

[L-02] It's not possible to remove previously added authority

PartyGovernanceNFT.sol implements only addAuthority() function. It allows to set any provided address as an authority. Only the party itself can add authorities.

function addAuthority(address authority) external onlySelf {
        isAuthority[authority] = true;

        emit AuthorityAdded(authority);
    }

However, there's no function which allows to remove authority. The only possible way of removing authority is calling abdicateAuthority(). However, this function allows to remove only own address (msg.sender) from the authority. In other words, if user X has authority, he/she can call abdicateAuthority() to remove himself/herself from the authority. However, there might be a scenario where party wants to remove user X from the authority, but X doesn't want that (thus he/she won't call abdicateAuthority()). In that case - it's not possible to remove X from authority - since protocol does not implement any function which allows removing others from authority.

[L-03] Possible overflow in `ETHCrowdfundBase.sol`

Field duration is defined as uint40:

uint40 duration;

which implies, that user may set duration to any amount within a range (0, type(uint40).max). Whenever user creates very long duration (so that duration + block.timestamp > type(uint40).max)) - the overflow will occur in the below line:

expiry = uint40(block.timestamp + opts.duration);

Now, when expiry overflows, the crowdfund will finish immediately:

if (block.timestamp >= expiry_) {

Since it's very unlikely that user will ever set such a huge duration - the severity of this issue has been evaluated as Low. Nonetheless, we do recommend to perform additional check which will verify if expiry hasn't overflown.

[L-04] Lack of address(0) check for `authorities` in `PartyGovernanceNFT.sol`

 for (uint256 i; i < authorities.length; ++i) {
                isAuthority[authorities[i]] = true;

When calling _initialize(), in parameter address[] memory authorities, we pass an array of addresses which becomes authority. There's no additional check which verifies if any of this address is address(0).

[L-05] Lack of value 0 check for `totalVotingPower` in `_initialize()` in `PartyGovernance.sol`

totalVotingPower: govOpts.totalVotingPower

There's no additional check which verifies if totalVotingPower was not set to 0 in function _initialize(). If it was - function _isUnanimousVotes() will always revert due to division by zero:

uint256 acceptanceRatio = (totalVotes * 1e4) / totalVotingPower;

The same division by zero revert may occur in function _areVotesPassing()

return (uint256(voteCount) * 1e4) / uint256(totalVotingPower) >= uint256(passThresholdBps);

Since setting this value to 0 in _initialize() is unreasonable and unlikely to occur, we've marked the severity of this issue as Low.

[L-06] Lack of validation of `fundingSplitBps` in `_initialize()` in `ETHCrowdfundBase.sol` may lead to revert due to underflow

 fundingSplitBps = opts.fundingSplitBps;

In function _initialize(), there should be additional check which requires fundingSplitBps to be smaller than 1e4. Otherwise, underflow may occur in following lines of code:

 fundingSplitBps = opts.fundingSplitBps;

 amount = (amount * (1e4 - fundingSplitBps_)) / 1e4;

 amount = (amount * 1e4) / (1e4 - fundingSplitBps_);

 totalContributions_ -= (totalContributions_ * fundingSplitBps_) / 1e4;

[L-07] Lack of casting to `uint256` may lead to overflow in `_isUnanimousVotes()` function in `PartyGovernance.sol`

uint256 acceptanceRatio = (totalVotes * 1e4) / totalVotingPower;

Since acceptanceRatio is uint256 and totalVotes is uint96, we can safely assume, that (totalVotes * 1e4) / totalVotingPower will never exceed type(uint256).max and the result will fit in uint256. However, the problem occurs with multiplication: since totalVotes is uint96 - (totalVotes * 1e4) will also be treated as uint96 - and this might overflow.

The safest way to avoid the risk of overflow, would be to cast totalVotes to uint256 before doing the multiplication:

uint256 acceptanceRatio = (uint256(totalVotes) * 1e4) / totalVotingPower;

[R-01] Standardize how `batchContribute()` and `batchContributeFor()` calculate `msg.value`

In InitialETHCrowdfund.sol there are two functions which allows to perform batch operation. Both of them, treat msg.value differently.

In batchContribute(), we assign msg.value to ethAvailable and divide it to each votingPowers passed as a parameter. If msg.value is bigger than a sum of provided votinPowers, we refund unused ETH:

if (ethAvailable > 0) payable(msg.sender).transfer(ethAvailable);

In batchContributeFor(), there's no potential refund, because this function requires that the sum of votingPowers is exactly the same as msg.value:

 if (msg.value != valuesSum) {
            revert InvalidMessageValue();
        }

Standardize how batch-like functions works. They both should either refund unused ETH (like batchContribute() does) or they both should require that provided sum of votingPowers is the same as msg.value (like batchContributeFor() does). Having similar functions which behave slightly differently, may lead to confusion to the end-user.

[R-02] Use 2-step Host Party transfer

Whenever user provides incorrect newPartyHost parameter, function abdicateHost() will transfer party host status to incorrect address. Since Party Host seems to be crucial role across the whole protocol design - a good idea would be to implement a 2-step Party Host transfer.

function abdicateHost(address newPartyHost) external {

[R-03] Define magic constant for `acceptanceRatio` in `PartyGovernance.sol`

File: ProposalExecutionEngine.sol

return acceptanceRatio >= 0.9999e4;

For better code readability, define additional constant for 0.9999e4 value.

[R-04] Use constants which represent values, instead of hard-coding numbers

Define constant for 1e4 and 1e18

PartyGovernance.sol:277:        if (govOpts.feeBps > 1e4) {
PartyGovernance.sol:280:        if (govOpts.passThresholdBps > 1e4) {
PartyGovernance.sol:1115:        uint256 acceptanceRatio = (totalVotes * 1e4) / totalVotingPower;
PartyGovernance.sol:1134:        return (uint256(voteCount) * 1e4) / uint256(totalVotingPower) >= uint256(passThresholdBps);
ETHCrowdfundBase.sol:266:            amount = (amount * (1e4 - fundingSplitBps_)) / 1e4;
ETHCrowdfundBase.sol:270:        votingPower = (amount * exchangeRateBps) / 1e4;
ETHCrowdfundBase.sol:281:        amount = (votingPower * 1e4) / exchangeRateBps;
ETHCrowdfundBase.sol:287:            amount = (amount * 1e4) / (1e4 - fundingSplitBps_);
ETHCrowdfundBase.sol:325:            totalContributions_ -= (totalContributions_ * fundingSplitBps_) / 1e4;
ETHCrowdfundBase.sol:329:        uint96 newVotingPower = (totalContributions_ * exchangeRateBps) / 1e4;
ETHCrowdfundBase.sol:355:        splitAmount = (totalContributions * fundingSplitBps_) / 1e4;
PartyGovernanceNFT.sol:157:            totalVotingPower == 0 ? 0 : (votingPowerByTokenId[tokenId] * 1e18) / totalVotingPower;
PartyGovernanceNFT.sol:393:                    withdrawAmounts[i] += (balance * getVotingPowerShareOf(tokenIds[j])) / 1e18;
PartyGovernanceNFT.sol:413:                uint256 fee = (amount * feeBps_) / 1e4;

[N-01] Missing reason strings in `require` statements

Require statements should have descriptive reason strings

        require(proposalType != ProposalType.Invalid);
        require(uint8(proposalType) <= uint8(type(ProposalType).max));

[N-02] Typos in comment

This typo was not detected during the bot-race

 // Fee recipeint for distributions.

Should be: Fee recipient for distributions

[N-03] Uncompleted comment in `InitialETHCrowdfund.sol`

// IDs of cards to credit the contributions to. When set to 0, it means
        uint256[] tokenIds;

Comment in InitialETHCrowdfund.sol is not finished. It seems that it was a copy&paste from BatchContributeForArgs (lines 79-80), and the last line has been cut. The full comment should be: IDs of cards to credit the contributions to. When set to 0, it means a new one should be minted.

[N-04] Incorrect naming convention for non-constants

It's a good coding practice to use uppercase to name constants. In PartyGovernance.sol and PartyGovernanceNFT.sol uppercase naming is used for immutable variables instead:

IGlobals private immutable _GLOBALS;

IGlobals private immutable _GLOBALS;

#0 - c4-pre-sort
2023-11-13T04:16:00Z

ydspa marked the issue as sufficient quality report

#1 - c4-judge
2023-11-19T18:12:50Z

gzeon-c4 marked the issue as grade-a

#2 - 0xble
2023-11-21T18:41:56Z

Will fix the following: L-01 L-03 L-04 L-07 R-01 R-03 N-02 N-03

#3 - c4-sponsor
2023-11-21T18:42:06Z

0xble (sponsor) confirmed

Findings Information

🌟 Selected for report: 0xVolcano

Also found by: 0x11singh99, 0xAnah, 0xhacksmithh, 0xhex, 0xta, DavidGiladi, K42, Topmark, arjun16, dharma09, hunter_w3b, lsaudit, pavankv, tabriz, tala7985, ybansal2403

Awards

23.8054 USDC - $23.81

Labels

bug

G (Gas Optimization)

grade-b

sufficient quality report

G-14

External Links

Link to GitHub issue

[G-01] Redundant variable in `OffChainSignatureValidator.sol`

In function isValidSignature() variable encodedPacket is used only once, which means it does not need to be declared at all. Code can be changed to:

if (keccak256(abi.encodePacked(
            "\x19Ethereum Signed Message:\n", Strings.toString(message.length), message)) != hash) {
            revert MessageHashMismatch();
        }

[G-02] Use constants instead of `type(uintX).max`

When you use type(uintX).max - it may result in higher gas costs because it involves a runtime operation to calculate the type(uintX).max at runtime. This calculation is performed every time the expression is evaluated.

To save gas, it is recommended to use constants to represent the maximum value. Declaration of a constant with the maximum value means that the value is known at compile-time and does not require any runtime calculations.

ProposalExecutionEngine.sol

185:	stor.nextProgressDataHash = bytes32(type(uint256).max);

PartyGovernance.sol

187:    uint96 private constant VETO_VALUE = type(uint96).max;
302:        if (govOpts.hosts.length > type(uint8).max) {
359:        return getVotingPowerAt(voter, timestamp, type(uint256).max);
445:        return high == 0 ? type(uint256).max : high - 1;
520:        emit BatchMetadataUpdate(0, type(uint256).max);
581:        emit BatchMetadataUpdate(0, type(uint256).max);
655:            emit BatchMetadataUpdate(0, type(uint256).max);
688:        emit BatchMetadataUpdate(0, type(uint256).max);
833:        emit BatchMetadataUpdate(0, type(uint256).max);
897:        emit BatchMetadataUpdate(0, type(uint256).max);
926:            if (hintIndex != type(uint256).max) {
1083:        if (pv.votes == type(uint96).max) {

[G-03] Redesign `decreaseVotingPower()` in `PartyGovernanceNFT.sol`

mintedVotingPower is increased every time increaseVotingPower() is called for any tokenId. votingPowerByTokenId[tokenId] is increased every time increaseVotingPower(tokenId, ...) is increased.

This allows us to assume, that mintedVotingPower >= votingPowerByTokenId[tokenId]

We can redesign decreaseVotingPower():

    function decreaseVotingPower(uint256 tokenId, uint96 votingPower) external {
        _assertAuthority();
        votingPowerByTokenId[tokenId] -= votingPower;
        unchecked { mintedVotingPower -= votingPower; }
        

        _adjustVotingPower(ownerOf(tokenId), -votingPower.safeCastUint96ToInt192(), address(0));
    }

If votingPower > votingPowerByTokenId[tokenId], function will revert (due to underflow). If function won't revert, then we know that votingPower <= votingPowerByTokenId[tokenId]. But we also know, that since mintedVotingPower >= votingPowerByTokenId[tokenId], thus mintedVotingPower -= votingPower won't revert too, thus this operation can be unchecked.

[G-04] Length of arrays are calculated multiple of times in `PartyGovernanceNFT.sol`

withdrawTokens.length is calculated three times in PartyGovernanceNFT.sol

375:	 uint256[] memory withdrawAmounts = new uint256[](withdrawTokens.length);
376:        {
377:            IERC20 prevToken;
378:            for (uint256 i; i < withdrawTokens.length; ++i) {
(...)
408:	 for (uint256 i; i < withdrawTokens.length; ++i) {

Calculating length of array is costly operation. It's better to calculate it once, cache the result in variable and then use later. During the bot-report, it was only mentioned, that withdrawTokens.length is calculated on every loop iteration and should be cached. However, it should be cached even sooner. It is used in line 375, 378, 408.

tokenIds.length is calculated two times in PartyGovernanceNFT.sol

350:	if (tokenIds.length == 0) revert NothingToBurnError();
(...)
391:	for (uint256 j; j < tokenIds.length; ++j) {

Calculating length of array is costly operation. It's better to calculate it once, cache the result in variable and then use later. During the bot-report, it was only mentioned, that tokenIds.length is calculated on every loop iteration and should be cached. However, it should be cached even sooner. It is used in line 350, 391.

[G-05] Use cached variable in `PartyGovernanceNFT.sol`

        mintedVotingPower += votingPower_;
        votingPowerByTokenId[tokenId] = votingPower_;

mintedVotingPower is already assigned to local variable uint96 mintedVotingPower_. Above code should be changed to:

        mintedVotingPower = mintedVotingPower_ + votingPower_;
        votingPowerByTokenId[tokenId] = votingPower_;

        mintedVotingPower += votingPower;
        uint256 newIntrinsicVotingPower = votingPowerByTokenId[tokenId] + votingPower;

mintedVotingPower is already assigned to local variable uint96 mintedVotingPower_. Above code should be changed to:

        mintedVotingPower = mintedVotingPower_ + votingPower;
        uint256 newIntrinsicVotingPower = votingPowerByTokenId[tokenId] + votingPower;

[G-06] Move condition which might revert quicker on top in `PartyGovernance.sol`

In function accept(), below condition can be moved on top:

 if (lastRageQuitTimestamp == block.timestamp) {
            revert CannotRageQuitAndAcceptError();
        }

If function revert with CannotRageQuitAndAcceptError() ealier, it will save gas for other more costly operations (such as getting the status of proposal).

[G-07] Cache `party` into memory variable in `InitialETHCrowdfund.sol`

party is being used multiple of times. It can be cached into memory variable:

        uint96 votingPower = party.votingPowerByTokenId(tokenId).safeCastUint256ToUint96();
        amount = convertVotingPowerToContribution(votingPower);

        if (amount > 0) {
            // Get contributor to refund.
            address payable contributor = payable(party.ownerOf(tokenId));

            // Burn contributor's party card.
            party.burn(tokenId);

Cache party like this: Party party_ = party; and use party_ instead of party.

[G-08] `_createParty()` in `InitialETHCrowdfund.sol` can be optimized

        uint256 authoritiesLength = opts.authorities.length + 1;
        address[] memory authorities = new address[](authoritiesLength);
        for (uint i = 0; i < authoritiesLength - 1; ++i) {
            authorities[i] = opts.authorities[i];
        }
        authorities[authoritiesLength - 1] = address(this);

Above code block can be optimized:

we don't need to calculate authoritiesLength - 1 on every loop iteration
we don't need to add 1 to authoritiesLength, just to subtract it later, we can add 1 at line 378 instead

        uint256 authoritiesLength = opts.authorities.length;
        address[] memory authorities = new address[](authoritiesLength + 1); 
        for (uint i = 0; i < authoritiesLength; ++i) {
            authorities[i] = opts.authorities[i];
        }
        authorities[authoritiesLength] = address(this);

[G-09] Use cached variable in `ETHCrowdfundBase.sol`

 uint96 refundAmount = newTotalContributions - maxTotalContributions;

maxTotalContributions is already cached into variable maxTotalContributions_ at line 228: uint96 maxTotalContributions_ = maxTotalContributions;. You should use cached variable instead:

uint96 refundAmount = newTotalContributions - maxTotalContributions_;

[G-10] Condition which might revert should be moved higher in `PartyGovernance.sol`

In function _initialize(), below check should be moved higher:

 if (govOpts.hosts.length > type(uint8).max) {
            revert TooManyHosts();
        }

If there are too many hosts provided, function will revert earlier, without wasting gas on initializing the proposal execution engine and setting the governance parameters.

[G-11] Operations which won't underflow/overflow in `PartyGovernance.sol` can be unchecked.

Decreasing number of hosts in PartyGovernance.sol can be unchecked

--numHosts;

Function abdicateHost() can be called by existing host only, thus we know that numHost > 0 and decreasing it can be unchecked.

Increasing lastProposalId can be unchecked. If this ever overflow, the whole protocol will be broken (it won't be possible to create new proposals)

proposalId = ++lastProposalId;

[G-12] There's no need to cast in `PartyGovernance.sol`

 revert ProposalCannotBeCancelledYetError(
                    uint40(block.timestamp),
                    uint40(cancelTime)

There's no reason for casting uint256 to uint40, just to include them in revert message. It's better to redesign ProposalCannotBeCancelledYetError() so that it accepts uint256.

[G-13] Redundant variables in `PartyGovernance.sol`

Variable partyDao is used only once, thus it doesn't need to be declared at all:

 address partyDao = _GLOBALS.getAddress(LibGlobals.GLOBAL_DAO_WALLET);
            if (msg.sender != partyDao) {

can be changed to:

if (msg.sender != _GLOBALS.getAddress(LibGlobals.GLOBAL_DAO_WALLET)) {

The same issue occurs at line 243-244.

[G-14] Length of arrays are calculated multiple of times in `PartyGovernance.sol`

govOpts.hosts.length is calculated three times in PartyGovernance.sol