Since buyToken function has no slippage checking, users can get less tokens than expected when they buy tokens directly

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Users can buy NontransferableERC20Token by calling buyToken function directly. At that time, the expected amount of tokens they will receive is determined based on current supply and their paying ether amount. But, due to some transactions(such as settleAuction or another user's buyToken) which is running in front of caller's transaction, they can get less token than they expected.

Proof of Concept

The VRGDAC always exponentially increase the price of tokens if the supply is ahead of schedule. Therefore, if another transaction of buying token is frontrun against a user's buying token transaction, the token price can arise than expected.

For instance, let's assume that ERC20TokenEmitter is initialized with following params:

• target price: 1 ether
• decay percent: 10 %
• per time unit: 10 ether

To avoid complexity, we will assume that the supply of token so far is consistent with the schedule. When alice tries to buy token with 5 ether, expected amount is calculated by getTokenQuoteForEther(5 ether) and the value is about 4.87 ether.
However, if Bob's transaction to buy tokens with 10 ether is executed before Alice, the real amount which Alice will receive is about 4.43 ether.

You can check result through following test:

    function testBuyTokenWithoutSlippageCheck() public {
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");

        vm.deal(address(alice), 100000 ether);
        vm.deal(address(bob), 100000 ether);

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        // expected amount of minting token when alice calls buyToken
        int256 expectedAmount = erc20TokenEmitter.getTokenQuoteForEther(5 ether);

        vm.startPrank(bob);
        // assume that bob calls buy token with 10 ether
        erc20TokenEmitter.buyToken{ value: 10 ether }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        vm.stopPrank();

        vm.startPrank(alice);
        // calculate the amount of tokens which alice will actually receive
        int256 realAmount = erc20TokenEmitter.getTokenQuoteForEther(5 ether);

        vm.stopPrank();

        emit log_string("Expected Amount: ");
        emit log_int(expectedAmount);
        emit log_string("Real Amount: ");
        emit log_int(realAmount);

        assertLt(realAmount, expectedAmount, "Alice should receive less than expected if Bob frontrun buyToken");
    }

Therefore, Alice will get about 0.44 ether less tokens than expected since there is no any checking of slippage in buyToken function.

Tools Used

VS Code

Recommended Mitigation Steps

Add slippage checking to buyToken function. This slippage checking should be executed only when the user calls buyToken function directly. In other words, it should not be executed when settleAuction calls buyToken function.

Assessed type

Other

[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 not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[rocketman-21]

this is intended and a consequence of how the VRGDA functions, when people buy tokens the price goes up if it is ahead of schedule

not ideal UX, but not going to fix for now

[c4-sponsor]

rocketman-21 (sponsor) acknowledged

[MarioPoneder]

Even though the increasing price is intended, it's state of the art to introduce a slippage parameter to protect users from receiving less than expected. Therefore, maintaining Medium severity seems appropriate.

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report