code423n4 commented 2 years ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L212-L226

Vulnerability details

Impact

If auction market finalization always reverts, the fund will be locked in the Crowdfund contract forever. Even if CrowdfundLifecycle is expired. This causes fund loss.

If CrowdfundLifecycle is expired and we cannot finalize the auction, we should mark the crowdfund as "Lost" and return the fund to contributors.

Proof of Concept

        if (lastBid_ != 0) {
            uint256 auctionId_ = auctionId;
            // Finalize the auction if it isn't finalized.
            if (!market.isFinalized(auctionId_)) {
                // Note that even if this crowdfund has expired but the auction is still
                // ongoing, this call can fail and block finalization until the auction ends.
                (bool s, bytes memory r) = address(market).call(abi.encodeCall(
                    IMarketWrapper.finalize,
                    auctionId_
                ));
                if (!s) {
                    r.rawRevert();
                }
            }
        } else {
            // If we never placed a bid, the auction must have expired.
            if (lc != CrowdfundLifecycle.Expired) {
                revert AuctionNotExpiredError();
            }
        }
...
        } else {
            // Clear `lastBid` so `_getFinalPrice()` is 0 and people can redeem their
            // full contributions when they burn their participation NFTs.
            lastBid = 0;
            emit Lost();
        }

        _bidStatus = AuctionCrowdfundStatus.Finalized;

If IMarketWrapper.finalize call always revert, it will always skip the block that set lastBid to 0 and _bidStatus to Finalized. Result in crowdfund never finalized and fund of contributors will be locked in the AuctionCrowdfund contract forever

Tools Used

Manual review

Recommended Mitigation Steps

You should create an emergency switch that can be toggled when both crowdfund is expired and IMarketWrapper.finalize revert. And check for that emergency switch if it is toggled and IMarketWrapper.finalize reverts, you should mark the crowdfund as lost ignoring fund lost due to a bug in the target marketplace.

...
            if (!market.isFinalized(auctionId_)) {
                // Note that even if this crowdfund has expired but the auction is still
                // ongoing, this call can fail and block finalization until the auction ends.
                (bool s, bytes memory r) = address(market).call(abi.encodeCall(
                    IMarketWrapper.finalize,
                    auctionId_
                ));
                if (!s && !emergencySwitch) {
                    r.rawRevert();
                }
            }
        } else {
...

merklejerk commented 2 years ago

We're OK with this risk. It's something we look out for and handle inside market wrappers.

HardlyDifficult commented 2 years ago

The report here seems theoretically sound. However without context on which market / scenario could put the party into this state it's hard to support the suggested risk. Downgrading to Low risk, as described this is an additional safegaurd for consideration.

Downgrading and converting into a QA report for the warden.

code-423n4 / 2022-09-party-findings