Users receive fewer tokens due to inaccuracy in calculation

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/libs/VRGDAC.sol#L57-L81

Vulnerability details

Impact

There is a loss of precision in the VRGDAC.yToX function, because in several places division occurs first, and then the result is multiplied. This results in users receiving fewer tokens. According to test data, the difference can be 8 digits. The choice of vulnerability severity takes into account the high probability of the event and the possibility of even more significant damage.

Proof of Concept

There are two places where division occurs first at the VRGDAC.yToX function. This can be easily fixed according to Recommended Mitigation Steps. The difference in calculation accuracy can be verified using an existing test ERC20TokenEmitterTest.testBuyingLaterIsBetter. The test requires adding logging of the amount of tokens.

    function testBuyingLaterIsBetter() public {
        vm.startPrank(address(0));

        int256 initAmount = erc20TokenEmitter.getTokenQuoteForEther(1e18);

        int256 firstPrice = erc20TokenEmitter.buyTokenQuote(1e19);

        // solhint-disable-next-line not-rely-on-time
        vm.warp(block.timestamp + (10 days));

        int256 secondPrice = erc20TokenEmitter.buyTokenQuote(1e19);

        emit log_int(firstPrice);
        emit log_int(secondPrice);

        int256 laterAmount = erc20TokenEmitter.getTokenQuoteForEther(1e18);

+       emit log_int(initAmount);
+       emit log_int(laterAmount);

        assertGt(laterAmount, initAmount, "Later amount should be greater than initial amount");

        assertLt(secondPrice, firstPrice, "Second price should be less than first price");
    }

Test result of the existing implementation:

@collectivexyz/revolution:test: [PASS] testBuyingLaterIsBetter() (gas: 77863)
@collectivexyz/revolution:test: Logs:
@collectivexyz/revolution:test:   10005269876410031943
@collectivexyz/revolution:test:   3488621893286168550
@collectivexyz/revolution:test:   999947323442167000
@collectivexyz/revolution:test:   2867538769039488000

Test result with changes made:

@collectivexyz/revolution:test: [PASS] testBuyingLaterIsBetter() (gas: 77863)
@collectivexyz/revolution:test: Logs:
@collectivexyz/revolution:test:   10005269876410031943
@collectivexyz/revolution:test:   3488621893286168550
@collectivexyz/revolution:test:   999947323442167590
@collectivexyz/revolution:test:   2867538769068152103

Tools Used

Manual review

Recommended Mitigation Steps

Concider usind division after multiplying in all places where it possible:

    function yToX(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) {
        int256 soldDifference = wadMul(perTimeUnit, timeSinceStart) - sold;
        unchecked {
            return
+               wadDiv(
                    wadMul(
                        perTimeUnit,
-                       wadDiv(                        
                        wadLn(
                            wadDiv(
                                wadMul(
                                    targetPrice,
                                    wadMul(
                                        perTimeUnit,
-                                       wadExp(wadMul(soldDifference, wadDiv(decayConstant, perTimeUnit)))
+                                       wadExp(wadDiv(wadMul(soldDifference, decayConstant), perTimeUnit))
                                    )
                                ),
                                wadMul(
                                    targetPrice,
                                    wadMul(
                                        perTimeUnit,
                                        wadPow(1e18 - priceDecayPercent, wadDiv(soldDifference, perTimeUnit))
                                    )
                                ) - wadMul(amount, decayConstant)
+                           )
                        )                        
                    ),
                    decayConstant
-                   )                     
                );
        }
    }

Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

QA likely. Medium at best.

[c4-sponsor]

rocketman-21 (sponsor) confirmed

[MarioPoneder]

QA seems most appropriate due to the governance token having 18 decimals vs. a shown rounding error of 8 decimals, therefore the rounding error is negligible. Moreover, no subsequent impacts are shown/proven.

QA findings which are originally submitted as High are judged as overinflated severity.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Overinflated severity