30973 - [SC - Low] Incorrect Validation of treasuryPct in the Reve...

Submitted on May 9th 2024 at 23:24:07 UTC by @The_Seraphs for Boost | Alchemix

Report ID: #30973

Report type: Smart Contract

Report severity: Low

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/RevenueHandler.sol

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

• Smart contract unable to operate due to lack of token funds

• Protocol insolvency

Description

Brief/Intro

The constructor of the RevenueHandler contract incorrectly checks an uninitialised state variable (treasuryPct) instead of the parameter (_treasuryPct). This issue could lead to setting an unintended treasury percentage without proper validation.

Note: I've set as a medium severity, due to setting an incorrect treasury percentage could lead to financial discrepancies in revenue distribution between the Treasury and veALCX holders - Ultimately affecting the protocol's economic model. However, I do understand that this is part of a constructor, so the likelihood is low and the setTreasuryPct() can be used to fix once noticed.

Vulnerability Details

Components Affected

• Contract Name: RevenueHandler

• Functionality: Constructor and Treasury Percentage Setup

Impact Details

The constructor of the RevenueHandler contract is intended to initialise the treasury percentage (treasuryPct) used to calculate the portion of revenues sent to the treasury. The code currently checks the uninitialised state variable treasuryPct instead of the input parameter _treasuryPct. The correct line should validate _treasuryPct as it's the input provided during contract deployment and affects subsequent financial calculations.

If not corrected, this bug could allow the initialisation of the RevenueHandler with a treasuryPct exceeding 100%. This could lead to errors in calculating the treasury's share of revenues, potentially resulting in economic losses or unintended distribution of funds.

Current implementation:

    constructor(address _veALCX, address _treasury, uint256 _treasuryPct) Ownable() {
        veALCX = _veALCX;
        require(_treasury != address(0), "treasury cannot be 0x0");
        treasury = _treasury;
        require(treasuryPct <= BPS, "treasury pct too large"); // incorrect check
        treasuryPct = _treasuryPct;
    }

Proposed fix:

    constructor(address _veALCX, address _treasury, uint256 _treasuryPct) Ownable() {
        veALCX = _veALCX;
        require(_treasury != address(0), "treasury cannot be 0x0");
        treasury = _treasury;
        require(_treasuryPct <= BPS, "treasury pct too large"); // corrected
        treasuryPct = _treasuryPct;

References

• Link to code (contract): https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/RevenueHandler.sol#L73C1-L79C6

• Link to code (test file): https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/test/BaseTest.sol#L151

Proof of Concept

• Using the existing src/test/RevenueHandler.t.sol test contract

• Add the following test function

    function testRevenueHandlerTreasuryPct() public {
        uint256 treasuryPct = revenueHandler.treasuryPct();
        console.log("Treasury percentage: %s", treasuryPct);
    }

• Go to the BaseTest.sol test contract and adjust setUp() which initiates the RevenueHandler with the required parameters.

        revenueHandler = new RevenueHandler(address(veALCX), admin, 50000); // @audit change to higher than BPT

• Run the test from the terminal forge test --mt testRevenueHandlerTreasuryPct -vv

Results:

[⠒] Compiling...
No files changed, compilation skipped

Ran 1 test for src/test/RevenueHandler.t.sol:RevenueHandlerTest
[PASS] testRevenueHandlerTreasuryPct() (gas: 10892)
Logs:
  Treasury percentage: 50000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.25s (70.92ms CPU time)

Ran 1 test suite in 6.62s (6.25s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)