c4-bot-6 commented 11 months ago

Lines of code

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

Vulnerability details

Impact

The price of governance tokens is controlled by the demand (sold tokens) whether it being ahead of scheduled rate or behind it; where the scheduled rate is the number of tokens to be sold per day, and if the amount of sold tokens per day is greater than this rate; then the price will be increased, and if the amount of sold tokens per day is less than the rate limit; then the price will be decreased to encourage users to buy the token while the price will never reach zero .
But it was noticed that after some time passing without any token put=rchase; the price will be dropped to reach zero, this will lead to user buying these governance tokes while paying the protocol fees only.

Proof of Concept

Code Instances:

ERC20TokenEmitter.buyTokenQuote function

    /**
     * @notice Returns the amount of wei that would be spent to buy an amount of tokens. Does not take into account the protocol rewards.
     * @param amount the amount of tokens to buy.
     * @return spentY The cost in wei of the token purchase.
     */
    function buyTokenQuote(uint256 amount) public view returns (int spentY) {
        require(amount > 0, "Amount must be greater than 0");
        // Note: By using toDaysWadUnsafe(block.timestamp - startTime) we are establishing that 1 "unit of time" is 1 day.
        // solhint-disable-next-line not-rely-on-time
        return
            vrgdac.xToY({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
                sold: emittedTokenWad,
                amount: int(amount)
            });
    }

Foundry PoC:

Add this testTokenPriceReachesZero fuzzing test inside the ERC20TokenEmitterTest test contract that resides in the ERC20TokenEmitter.t.sol test file which is located in the following directory packages/revolution/test/token-emitter/ERC20TokenEmitter.t.sol :

   ├── ERC20TokenEmitter.t.sol
   │   ├── ERC20TokenEmitterTest
   │       ├── testTokenPriceReachesZero

    function testTokenPriceReachesZero(uint256 _daysElapsed) public {
        address buyer = address(48);

        //1. buy 1 ether of tokens to increase the totsalSupply:
        address[] memory recipients = new address[](1);
        recipients[0] = buyer;
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        uint256 paymentAmount = 1 ether;
        vm.deal(buyer, paymentAmount);

        vm.startPrank(buyer);
        erc20TokenEmitter.buyToken{ value: paymentAmount }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({ builder: address(0), purchaseReferral: address(0), deployer: address(0) })
        );

        //2. after days pass without token purchase, the price should never reach zero, but it was noticed that it will reach zero after some time:
        uint256 amountOfTokensToPurchase = 1e18;
        vm.warp(block.timestamp + _daysElapsed * 1 days);

        // The price will reach zero after 317days without purchase:
        int256 priceAfterManyDays = erc20TokenEmitter.buyTokenQuote(amountOfTokensToPurchase);
        if (priceAfterManyDays == 0) {
            console.log("days elapsed after which the price reaches zero : ", _daysElapsed);
        }
        assertGt(priceAfterManyDays, 0, "Price should never hit zero");
    }

Explained scenario:
1. first some governance tokens were bought for 1ether of payment.
2. then days pass without any token purchase.
3. the token price decreases until it hits zero (the assertion of token price will revert).
Test result:

$ FOUNDRY_PROFILE=dev forge test --match-test testTokenPriceReachesZero -vvv
Running 1 test for test/token-emitter/ERC20TokenEmitter.t.sol:ERC20TokenEmitterTest
[FAIL. Reason: Assertion failed. Counterexample: calldata=0xe9c703dd000000000000000000000000000000000000000000000000000000000000013f, args=[319]] testTokenPriceReachesZero(uint256) (runs: 4, μ: 321138, ~: 321138)
Logs:
  days elapsed after which the price reaches zero :  319
  Error: Price should never hit zero
  Error: a > b not satisfied [int]
    Value a: 0
    Value b: 0

Traces:
  [341238] ERC20TokenEmitterTest::testTokenPriceReachesZero(319)
    ├─ [0] VM::deal(0x0000000000000000000000000000000000000030, 1000000000000000000 [1e18])
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000000030)
    │   └─ ← ()
    ├─ [285881] ERC20TOKEN_EMITTER::buyToken{value: 1000000000000000000}([0x0000000000000000000000000000000000000030], [10000 [1e4]], (0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000))
    │   ├─ [280847] ERC20TokenEmitter::buyToken([0x0000000000000000000000000000000000000030], [10000 [1e4]], (0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000)) [delegatecall]
    │   │   ├─ [30120] RevolutionProtocolRewards::depositRewards{value: 25000000000000000}(REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], 10000000000000000 [1e16], REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], 5000000000000000 [5e15], REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], 2500000000000000 [2.5e15], REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], 7500000000000000 [7.5e15])
    │   │   │   ├─ emit RewardsDeposit(builderReferral: REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], purchaseReferral: REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], deployer: REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], revolution: REVOLUTION_DAO: [0x0376AAc07Ad725E01357B1725B5ceC61aE10473c], from: ERC20TOKEN_EMITTER: [0xC56CEAb249b7Cdd702Fbc59349fEa9df1EB56Bc3], builderReferralReward: 10000000000000000 [1e16], purchaseReferralReward: 5000000000000000 [5e15], deployerReward: 2500000000000000 [2.5e15], revolutionReward: 7500000000000000 [7.5e15])
    │   │   │   └─ ← ()
    │   │   ├─ [6172] VRGDAC::yToX(0, 0, 975000000000000000) [staticcall]
    │   │   │   └─ ← 974949924259285000
    │   │   ├─ [5035] DAO::fallback{value: 975000000000000000}()
    │   │   │   ├─ [55] VerbsDAOLogicV1::receive() [delegatecall]
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ [154004] ERC20TOKEN::mint(0x0000000000000000000000000000000000000030, 974949924259285000 [9.749e17])
    │   │   │   ├─ [149015] NontransferableERC20Votes::mint(0x0000000000000000000000000000000000000030, 974949924259285000 [9.749e17]) [delegatecall]
    │   │   │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000030, value: 974949924259285000 [9.749e17])
    │   │   │   │   ├─ emit DelegateVotesChanged(delegate: 0x0000000000000000000000000000000000000030, previousVotes: 0, newVotes: 974949924259285000 [9.749e17])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit PurchaseFinalized(buyer: 0x0000000000000000000000000000000000000030, payment: 1000000000000000000 [1e18], treasuryAmount: 975000000000000000 [9.75e17], protocolRewardsAmount: 25000000000000000 [2.5e16], buyerTokensEmitted: 974949924259285000 [9.749e17], creatorTokensEmitted: 0, creatorDirectPayment: 0)
    │   │   └─ ← 974949924259285000 [9.749e17]
    │   └─ ← 974949924259285000 [9.749e17]
    ├─ [0] VM::warp(27561601 [2.756e7])
    │   └─ ← ()
    ├─ [14537] ERC20TOKEN_EMITTER::buyTokenQuote(1000000000000000000 [1e18]) [staticcall]
    │   ├─ [14048] ERC20TokenEmitter::buyTokenQuote(1000000000000000000 [1e18]) [delegatecall]
    │   │   ├─ [11901] VRGDAC::xToY(319000000000000000000, 974949924259285000, 1000000000000000000) [staticcall]
    │   │   │   └─ ← 0
    │   │   └─ ← 0
    │   └─ ← 0
    ├─ [0] console::9710a9d0(0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000013f00000000000000000000000000000000000000000000000000000000000000326461797320656c6170736564206166746572207768696368207468652070726963652072656163686573207a65726f203a200000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ emit log_named_string(key: Error, val: Price should never hit zero)
    ├─ emit log(: Error: a > b not satisfied [int])
    ├─ emit log_named_int(key:   Value a, val: 0)
    ├─ emit log_named_int(key:   Value b, val: 0)
    ├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   └─ ← ()
    └─ ← ()

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 102.55ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/token-emitter/ERC20TokenEmitter.t.sol:ERC20TokenEmitterTest
[FAIL. Reason: Assertion failed. Counterexample: calldata=0xe9c703dd000000000000000000000000000000000000000000000000000000000000013f, args=[319]] testTokenPriceReachesZero(uint256) (runs: 4, μ: 321138, ~: 321138)

Encountered a total of 1 failing tests, 0 tests succeeded

So as can be noticed from the test result above; the price will hit zero after 319 days with a total sold tokens of 1 ether and no purchases were made after that, note that the number of days where the token reaches zero depends on the total number of sold tokens (emittedTokenWad).

Tools Used

Manual Review & Foundry.

Recommended Mitigation Steps

Update vrgdac related functions to prevent returning zero price, or implement a minimum price if it hits zero.

Assessed type

Context

c4-pre-sort commented 11 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #404

c4-judge commented 10 months ago

MarioPoneder marked the issue as unsatisfactory: Invalid

c4-judge commented 10 months ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 10 months ago

MarioPoneder marked the issue as grade-c

DevHals commented 10 months ago

Hi @MarioPoneder,

This issue is marked as a duplicate of 404 which indicates frontrunning effect on the governance token price, while this issue is not a duplicate; it doesn't share the root cause of the primary issue!
The price of the governance token should never reach zero, this is an invariant that shouldn't be broken at any case or circumstances, but the issue clarifies that this invariant will be broken in some cases.
This issue is more of a duplicate of issue #371

I kindly ask you to take a second look and re-evaluate this issue,

Thanks!

MarioPoneder commented 10 months ago

Thank you for your comment!

I partially agree. However, the price running down to 0 in case of no buy pressure over a long period of time itself is intended behavior and not breaking any invariant.
The core issue is that a 0 price will DoS the ERC20TokenEmitter contract which was not stated in this report.

DevHals commented 10 months ago

Hi @MarioPoneder ,,

The token price reaching zero after long time with no purchases is not intended by design, and this is supposed to be ensured and tested by the protocol team, quoting testGetTokenPrice that is wriiten by the protocol team to check that the price would never reach zero:

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

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

        int256 priceAfterManyPurchases = erc20TokenEmitter.buyTokenQuote(1e18);
        emit log_int(priceAfterManyPurchases);

        // Simulate the passage of time
        uint256 daysElapsed = 221;
        vm.warp(block.timestamp + daysElapsed * 1 days);

        int256 priceAfterManyDays = erc20TokenEmitter.buyTokenQuote(1e18);

        emit log_int(priceAfterManyDays);

        // Assert that the price is greater than zero
        assertGt(priceAfterManyDays, 0, "Price should never hit zero");
    }

and this issue shows a case in which the price would reach zero.

Also the root cause of this issue and issue 371 is that the vrgdac.xToY that's called when buyTokenQuote is invoked is malfunctioned and would result in:
1. returning zero price after some time (as stated in this issue).
2. and when some time passed after returning zero price ; it would revert , causing DoS of the ERC20TokenEmitter contract (as stated in issue 371).
- So this issue pointed to one effect of the flawed buyTokenQuote , and issue 371 pointed to the other effect on the protocol (both issues sharing the same root cause).
Would appreciate it if you could have a second look and re-evaluate it!

Thanks again for your time and consideration,,

MarioPoneder commented 10 months ago

Thank you for your comment!

I acknowlege that 0 price should not be reached after your proof. However, the subsequential DoS scenario was not stated in the original report which would make it eligible for duplication to #371. Therefore, QA still seems most appropriate.

Thank you for your understanding!

code-423n4 / 2023-12-revolutionprotocol-findings

`ERC20TokenEmitter` contract: the price of governance token can reach zero after some point of time without purchasing #601

Lines of code

Vulnerability details

Impact

Proof of Concept

Code Instances:

Foundry PoC:

Tools Used

Recommended Mitigation Steps

Assessed type