`buyToken` can be front-runned to increase ERC20 token price which results in lower amount minted to creators

[c4-bot-9]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #26

[c4-pre-sort]

raymondfam marked the issue as duplicate of #397

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[c4-judge]

MarioPoneder marked the issue as satisfactory