Submitted by osmanozdemir1, also found by rouhsamad, KupiaSec, deepplus, BARW, 0xDING99YA, bart1e, SpicyMeatball, DanielArmstrong, haxatron, Brenzee, and rvierdiiev
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L180 
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L184 
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L200-L215
Users can buy governance tokens with the ERC20TokenEmitter::buyToken() function. In this protocol, governance token prices are determined by a linear VRGDA calculation. The token price increases if the token supply is ahead of the schedule and decreases if it is behind the schedule.
There are also some other parameters I need to mention, which are creatorRateBps and entropyRateBps.
For example, if the creator rate is 10%, the entropy rate is 50%, and a user wants to buy 100 ETH worth of tokens:
- Creator cut is 10 ETH
- 5 ETH will be sent directly to the creator
- The other 5 ETH will be used to buy governance tokens for the creator.
- The user will get 90 ETH worth of governance tokens.
This is the main logic in the ERC20TokenEmitter contract.
Now, lets examine the buyToken function:
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L152C3-L230C6
This function:
Now lets check the getTokenQuoteForEther function:
As we can see in this function, how many governance tokens will be minted is calculated according to the current supply, which is emittedTokenWad parameter.
Alright, what is the issue?
The issue is that both of the totalTokensForCreators and totalTokensForBuyers parameters are separately calculated like they are independent of each other, and the emittedTokenWad is updated after that. However, this situation breaks the VRGDA logic and more tokens are minted for the same amount of ETH.
These two calculations should not be independent. Total governance tokens to mint for the total ETH payment should be calculated first, and then the governance token amounts should be proportionally distributed.
Calculating proportional ETH amounts for the buyer and the creator first, and then determining the corresponding governance tokens to mint separately, leads to a higher total number of governance tokens to be minted compared to calculating the total governance tokens required based on the overall ETH amounts to be paid.
Coded PoC
Down below you can find a basic PoC that proves calculating tokens to mint separately results in many more tokens than calculating it only once with total payment.
You can use the protocols own setup to run this PoC
-Copy and paste the snippet into the ERC20TokenEmitter.t.sol test file
-Run it with forge test --match-test testTokenQuoteForEther_is1plus1equal2 -vvv
Results after running:
Foundry
There are two ways to resolve this issue. An unfair but much easier one, and the fair but not that easy one
Unfair One:
The protocol team should decide which side (creators or buyers) will suffer from the unfairness, and update the emittedTokenWad in the middle of two calculations. The latter calculation will buy tokens at a higher price.
In the example below, the change is in favour of the creators. Buyers get tokens with updated prices to keep the VRGDA logic intact.
Fair One:
This one requires a little more work but it is more fair. The function should:
Example:
rocketman-21 (Revolution) confirmed
