Revolution Protocol - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L287-L291 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L348-L356

Vulnerability details

Impact

The reservePrice parameter can be changed during an ongoing auction, this can cause problems as if the owner sets this parameter to be greater than the current auction contract ETH balance this will lead to the burning of the auctionned token which was supposed to go to the highest bidder of the ongoing auction.

Proof of Concept

The issue can arise because the owner can change the reservePrice parameter during an ongoing auction by calling setReservePrice.

If the owner sets the reservePrice to be greater than the current auction contract ETH balance then even if the highest bidder bid was greater than the previous reservePrice amount, he will not receive the NFT as it would be burned according to the logic in the _settleAuction() :

if (address(this).balance < reservePrice) {
    // If contract balance is less than reserve price, refund to the last bidder
    if (_auction.bidder != address(0)) {
        _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
    }

    // And then burn the Noun
    verbs.burn(_auction.verbId);
}

This will not be fair for the highest bidder who did provide the sufficient ETH amount to win the auction but will not receive the NFT because the reservePrice was changed.

Tools Used

Manual review

Recommended Mitigation Steps

The reservePrice should only be changed when there is no auction ongoing and the contract is paused.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/ERC20TokenEmitter.sol#L152-L230 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/ERC20TokenEmitter.sol#L254-L264

Vulnerability details

Impact

The buyToken function can be called by anyone, an attacker can call this function to front run the transaction of a honest user who's trying to buy ERC20 tokens and increase the VRGDA price, thus when the user transaction goes through he will be minted less amount of ERC20 tokens than what was intended for his provided ether amount.

Proof of Concept

The issue occurs in the buyToken method shown below :

function buyToken(
    address[] calldata addresses,
    uint[] calldata basisPointSplits,
    ProtocolRewardAddresses calldata protocolRewardsRecipients
) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
    //prevent treasury from paying itself
    require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

    require(msg.value > 0, "Must send ether");
    // ensure the same number of addresses and bps
    require(addresses.length == basisPointSplits.length, "Parallel arrays required");

    // Get value left after protocol rewards
    uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
        msg.value,
        protocolRewardsRecipients.builder,
        protocolRewardsRecipients.purchaseReferral,
        protocolRewardsRecipients.deployer
    );

    //Share of purchase amount to send to treasury
    uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

    //Share of purchase amount to reserve for creators
    //Ether directly sent to creators
    uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;

    //@audit using getTokenQuoteForEther to get ERC20 tokens amount
    //Tokens to emit to creators
    int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
        ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
        : int(0);

    // Tokens to emit to buyers
    int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

    //Transfer ETH to treasury and update emitted
    emittedTokenWad += totalTokensForBuyers;
    if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

    //Deposit funds to treasury
    (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0));
    require(success, "Transfer failed.");

    //Transfer ETH to creators
    if (creatorDirectPayment > 0) {
        (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
        require(success, "Transfer failed.");
    }

    //Mint tokens for creators
    if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
        _mint(creatorsAddress, uint256(totalTokensForCreators));
    }

    uint256 bpsSum = 0;

    //Mint tokens to buyers

    for (uint256 i = 0; i < addresses.length; i++) {
        if (totalTokensForBuyers > 0) {
            // transfer tokens to address
            _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
        }
        bpsSum += basisPointSplits[i];
    }

    require(bpsSum == 10_000, "bps must add up to 10_000");

    emit PurchaseFinalized(
        msg.sender,
        msg.value,
        toPayTreasury,
        msg.value - msgValueRemaining,
        uint256(totalTokensForBuyers),
        uint256(totalTokensForCreators),
        creatorDirectPayment
    );

    return uint256(totalTokensForBuyers);
}

As it can be seen from the code above when buyToken is called it internally invokes the getTokenQuoteForEther function to find out the amount of ERC20 tokens to be minted given the ETH amount, getTokenQuoteForEther under the hood calls the VRGDAC yToX method to get the current amount of ERC20 tokens that would be emitted for an amount of wei:

return
    vrgdac.yToX({
        timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
        sold: emittedTokenWad,
        amount: int(etherAmount)
    });

Because the amount of ERC20 tokens to minted depends on the amount already minted and the emission time, an attacker can front-run a honest user call to buyToken to mint a certain amount of ERC20 tokens and when the user transaction goes through he will be minted less amount of ERC20 tokens than what was intended as the attacker has changed the price of the VRGDAC. This attack is similaire to slippage that can occur in the swap operations in decentralized AMMs.

To illustrate this issue let's take this example:

• Bob calls buyToken to mint an amount of ERC20 tokens and provides msg.value = 1 ether and expecting to get the correct amount of ERC20 tokens based on the current price.

• Alice sees Bob transaction and wants to grief him so she calls buyToken before him (front run) and mints a certain amount of ERC20 tokens.

• After Alice tx goes through the ERC20 emitted amount has now changed (increased) and thus the VRGDAC price is also changed (increases).

• When Bob tx is processed now, he will receive less ERC20 tokens for his provided ether amount because getTokenQuoteForEther will use in its calculation the current ERC20 emitted amount emittedTokenWad which was increased by Alice's transaction.

• And thus Alice has succeed in griefing Bob and forced him to take a loss on his ETH amount.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue an additional slippage parameters should be included in the buyToken function which allows the caller to set the minimum ERC20 tokens that he's expecting to receive.

Assessed type

Context