Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The VoterUpgradeableV2.createV3Gauge function incorrectly uses v2GaugeFactory instead of v3GaugeFactory #17

Open
c4-bot-8 opened this issue Sep 25, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue M-02 🤖_primary AI based primary recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/core/VoterUpgradeableV2.sol#L324

Vulnerability details

Impact

The gauges for the V3 pool are managed incorrectly by v2GaugeFactory rather than v3GaugeFactory.

Proof of Concept

In the VoterUpgradeableV2.createV3Gauge function, v2GaugeFactory is used instead of the appropriate v3GaugeFactory.

File: contracts\core\VoterUpgradeableV2.sol
323:         externalBribe = IBribeFactory(bribeFactoryCache).createBribe(token0, token1, string.concat("Fenix Bribes: ", symbol));
324:         gauge = IGaugeFactory(v2GaugeFactory).createGauge(
325:             token,
326:             votingEscrow,
327:             pool_,
328:             address(this),
329:             internalBribe,
330:             externalBribe,
331:             true,
332:             feeVault
333:         );

As a result, v2GaugeFactory manages the gauges for the V3 pool instead of v3GaugeFactory. The GaugeFactoryUpgradeable contract includes the defaultBlastGovernor and merklGaugeMiddleman variables, and the createGauge function initializes the gauge using these variables.

File: contracts\gauges\GaugeFactoryUpgradeable.sol
    function createGauge(
        address _rewardToken,
        address _ve,
        address _token,
        address _distribution,
        address _internal_bribe,
        address _external_bribe,
        bool _isDistributeEmissionToMerkle,
        address _feeVault
    ) external virtual override returns (address) {
        require(msg.sender == voter || msg.sender == owner(), "only voter or owner");

        address newLastGauge = address(new GaugeProxy());
        IGauge(newLastGauge).initialize(
            defaultBlastGovernor,
            _rewardToken,
            _ve,
            _token,
            _distribution,
            _internal_bribe,
            _external_bribe,
            _isDistributeEmissionToMerkle,
            merklGaugeMiddleman,
            _feeVault
        );

        last_gauge = newLastGauge;

        return newLastGauge;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to change the code in the createV3Gauge function as following:

-       gauge = IGaugeFactory(v2GaugeFactory).createGauge(
+       gauge = IGaugeFactory(v3GaugeFactory).createGauge(
            token,
            votingEscrow,
            pool_,
            address(this),
            internalBribe,
            externalBribe,
            true,
            feeVault
        );

Assessed type

Other

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 25, 2024
c4-bot-5 added a commit that referenced this issue Sep 25, 2024
@c4-bot-11 c4-bot-11 added the 🤖_primary AI based primary recommendation label Sep 25, 2024
@b-hrytsak
Copy link

Thank you for your submission.

The problem is valid. Although there are some mitigations, as the implementations of v2/v3 factories, gauges are the same and it would not have led to any consequences at first, it is more of a flexibility for the future, regarding possible updates.

This submission is valid, it also seems to be Overseverity to the c4r description of problem severity

Thank you for participation

@b-hrytsak b-hrytsak added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 30, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 30, 2024
@c4-judge
Copy link

alcueca changed the severity to 2 (Med Risk)

@c4-judge
Copy link

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 30, 2024
@c4-judge
Copy link

alcueca marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Sep 30, 2024
@C4-Staff C4-Staff added the M-02 label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue M-02 🤖_primary AI based primary recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants