Revolution Protocol - BARW's results

Lines of code

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

Vulnerability details

Impact

When buyingTokens, part of the received ETH is not send to the treasury but remains in the ERC20TokenEmitter contract. Since this contract has no way to withdraw ETH from it, the ETH is lost forever

Proof of Concept

When calling ERC20TokenEmitter::buyToken to buy new tokens, the msg.value of the transaction is used as the basis to determin how much tokens will be minted.

First, a total of 2,5% of fees are deducted for builder_rewards, purchas_referral, deployer_reward and revolution_reward and send to protocolRewards:

protocolRewards.depositRewards{ value: totalReward }(
   builderReferral,
   settings.builderReferralReward,
   purchaseReferral,
   settings.purchaseReferralReward,
   deployer,
   settings.deployerReward,
   revolutionRewardRecipient,
   settings.revolutionReward
);

The rest of the eth is divided into toPayTreasury, creatorDirectPayment and partToBuyTokensForCreator .

toPayTreasury, is send to the treasury:

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

creatorDirectPayment is send to the creator:

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

The problem arises from the fact that partToBuyTokensForCreator is used to calculate the amount of tokens to mint to the creator but is never send to the treasury and therefore remains in the ERC20TokenEmitter contract.

Since there is no function that can withdraw access ETH from the contract, the ETH is stuck and lost for ever.

Here is a POC to show this. It can be copied into ERC20TokenEmitter.t.sol and run there:

    function test_TokenEmitterBalance(uint256 creatorRateBps, uint256 entropyRateBps) public {
        // Assume valid rates
        vm.assume(creatorRateBps > 0 && creatorRateBps <= 10000 && entropyRateBps > 0 && entropyRateBps <= 10000);

        vm.startPrank(erc20TokenEmitter.owner());
        //set creatorRate and entropyRate
        erc20TokenEmitter.setCreatorRateBps(creatorRateBps);
        erc20TokenEmitter.setEntropyRateBps(entropyRateBps);
        vm.stopPrank();

        //expect tokenEmitter balance to start out at 0
        assertEq(address(erc20TokenEmitter).balance, 0, "Balance should start at 0");

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);

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

        erc20TokenEmitter.buyToken{ value: 1 ether }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        //assert that tokenEmitter balance is 0
        assertEq(
            uint(address(erc20TokenEmitter).balance),
            0,
            "TokenEmitter should have a balance of 0"
        );
    }

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Make sure to send the ETH used for buying tokens for the creator to the treasury.

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

When buying tokens through ERC20TokenEmitter::buyToken, tokens are been minted twice, once for the buyer and once for the creator. The problem is that for determining the amount of tokens to be minted for buyer and creator, the function getTokenQuoteForEther is called twice using the same emittedTokenWad as a basis. Since the price per token (or the amount of tokens one gets for a specific ETH amount), heavily depends on the current emittedTokenWad, using two quotes with the same emittedTokenWad and two different ETH amounts instead of one quote with the combined ETH amount results in more tokens to be minted.

Proof of Concept

The revolution protocol has a specific emissions target for the voting tokens (1000 tokens per day). Depending on how many tokens have already been sold until the moment one wants to purchase new tokens, the price of the token increases (if there are to many tokens sold already) or decreases (if currently there are too little tokens sold) from a given target price. To calculate the current price of the tokens, a VRGDAC model is used. Its results are based on the amount of ETH one wants to invest and the number of tokens already minted, tracked in the variable emittedTokenWad.
To prevent big buy of tokens for a fixed low price the VRGDAC model considers how much ETH one wants to invest and makes each additional token bought more expensive than the last one. Meaning that if you invest 1 ETH at time t1 and get x tokens, for investing 2 ETH at t1 you would get less than 2x tokens, incentivising you to only buy tokens up to the protocols emission target.

The problem that arises when calling ERC20TokenEmitter::buyToke is that there are two buy happening, one for the buyer themselves and one for the creator. Since both buys/mints happen in the same transaction they should be seen as one buy/mint and the amount of total tokens to be minted should be quoted once. The problem is that getTokenQuoteForEther to determine the amount of tokens to be minted is called twice, once for the buyer with the buyer ETH amount and once for the creator with the creator ETH amount. Since both quotes are using the same emittedTokenWad the total tokens minted are more than they should be if one quote for the buyer ETH amount + creator ETH amount was given.

Example: For simplicity we assume that the buyer ETH amount and the creator ETH amount are each 1 ETH.

When calling getTokenQuoteForEther for 1 ETH at the newly set up token, the result of the quote is 999947323442167000 tokens resulting in the mint of 2 x 999947323442167000 = 1999894646884334000. Calling getTokenQuoteForEther for 2 ETH results in 1999789308566249000 minted tokens, 105338318085000 less than for the first quote.

Here is a POC to show this. To run it copy it into ERC20TokenEmitter.t.sol and add import "forge-std/console.sol"; to the imports:

    function test_tokensToMint(uint256 creatorRateBps, uint256 entropyRateBps) public {
        // Assume valid rates
        vm.assume(creatorRateBps > 0 && creatorRateBps <= 10000 && entropyRateBps > 0 && entropyRateBps <= 10000);

        vm.startPrank(erc20TokenEmitter.owner());
        //set creatorRate and entropyRate
        erc20TokenEmitter.setCreatorRateBps(creatorRateBps);
        erc20TokenEmitter.setEntropyRateBps(entropyRateBps);
        vm.stopPrank();

        //quote for 1 eth

        int256 quote2Times1ETH = erc20TokenEmitter.getTokenQuoteForEther(1 ether);      
        int256 quote1Time2ETH = erc20TokenEmitter.getTokenQuoteForEther(2 ether);   
        
        console.log("Quote for 1 times 1 ether");
        console.logInt(quote2Times1ETH);
        console.log("Quote for 2 times 1 ether");
        console.logInt(2 * quote2Times1ETH);
        console.log("Quote for 1 times 2 ether");
        console.logInt(quote1Time2ETH);
        console.log("Differance between quotes");
        console.logInt(2 * quote2Times1ETH- quote1Time2ETH);

        //check that the quotes are the same
        assertEq(quote2Times1ETH, quote1Time2ETH, "The quoted tokens should be the same" );

    }

Tools Used

Manual review

Recommended Mitigation Steps

To avoid minting to many tokens when calling ERC20TokenEmitter::buyToke consider determining the total tokens that should be minted by calling getTokenQuoteForEther with the total value of buyer ETH amount + creator ETH amount and splitting the amount of tokens between buyer and creator afterwards.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When a user invokes ERC20TokenEmitter::buyToken, the calculation is erroneous, resulting in an inflated emittedTokenWad. Consequently, this inflation will cause an elevated price with vrgdac::yToX for any future token that will be minted.

Proof of Concept

Due to rounding down when minting tokens to multiple buyers in one transaction, less tokens are minted than initially caclulated by calling getTokenQuoteForEther But since the initially calculated value is added to ``emittedTokenWad`, its value will be higher than it should be making any future tokens more expensice than they should be.

Add this function to ERC20TokenEmitterTest

    function testBuyTokenCalculation() public {
        uint256 length = 5;

        address[] memory recipients = new address[](length);
        recipients[0] = address(1);
        recipients[1] = address(2);
        recipients[2] = address(3);
        recipients[3] = address(4);
        recipients[4] = address(5);


        uint256[] memory bps = new uint256[](length);
        bps[0] = 2433;
        bps[1] = 2567;
        bps[2] = 1111;
        bps[3] = 1444;
        bps[4] = 2445;


        vm.deal(address(0), 100000 ether);

        vm.startPrank(address(0));

        int256 emittedTokenWadBefore = erc20TokenEmitter.emittedTokenWad();
        // console.log("emittedTokenWad before buyToken: ",emittedTokenWadBefore);
        emit log_named_int("emittedTokenWad before buyToken", emittedTokenWadBefore);

        uint256 sendAmount = erc20TokenEmitter.buyToken{ value: 1e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );
        emit log_named_int("emittedTokenWad after buyToken: ",erc20TokenEmitter.emittedTokenWad());
        int emittedTokenWadShouldBe = emittedTokenWadBefore + int(erc20TokenEmitter.totalSupply());
        emit log_named_int("emittedTokenWad should be after buyToken: ",emittedTokenWadShouldBe);

        assertEq(emittedTokenWadShouldBe,erc20TokenEmitter.emittedTokenWad(), "emittedTokenWad number is wrong");
    }

Output

[FAIL. Reason: assertion failed] testBuyTokenAMountIsRight() (gas: 623866)
Logs:
  emittedTokenWad before buyToken: 0
  emittedTokenWad after buyToken: : 974949924259285000
  emittedTokenWad should be after buyToken: : 974949924259284998
  Error: emittedTokenWad number is wrong
  Error: a == b not satisfied [int]
    Expected: 974949924259284998
      Actual: 974949924259285000

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.85ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Tools Used

foundry

Recommended Mitigation Steps

Record how much token protocol send to each buyer and sum the values up.

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

Assessed type

Math