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

Minter.sol#startInflation() can be bypassed #99

Open
code423n4 opened this issue Jun 3, 2022 · 1 comment
Open

Minter.sol#startInflation() can be bypassed #99

code423n4 opened this issue Jun 3, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L104-L108

Vulnerability details

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L104-L108

    function startInflation() external override onlyGovernance {
        require(lastEvent == 0, "Inflation has already started.");
        lastEvent = block.timestamp;
        lastInflationDecay = block.timestamp;
    }

As lastEvent and lastInflationDecay are not initialized in the constructor(), they will remain to the default value of 0.

However, the permissionless executeInflationRateUpdate() method does not check the value of lastEvent and lastInflationDecay and used them directly.

As a result, if executeInflationRateUpdate() is called before startInflation():

  1. L190, the check of if _INFLATION_DECAY_PERIOD has passed since lastInflationDecay will be true, and initialPeriodEnded will be set to true right away;
  2. L188, since the lastEvent in totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent)); is 0, the totalAvailableToNow will be set to totalAvailableToNow ≈ currentTotalInflation * 52 years, which renders the constrains of totalAvailableToNow incorrect and useless.

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L115-L117

    function executeInflationRateUpdate() external override returns (bool) {
        return _executeInflationRateUpdate();
    }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L187-L215

    function _executeInflationRateUpdate() internal returns (bool) {
        totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
        lastEvent = block.timestamp;
        if (block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD) {
            currentInflationAmountLp = currentInflationAmountLp.scaledMul(annualInflationDecayLp);
            if (initialPeriodEnded) {
                currentInflationAmountKeeper = currentInflationAmountKeeper.scaledMul(
                    annualInflationDecayKeeper
                );
                currentInflationAmountAmm = currentInflationAmountAmm.scaledMul(
                    annualInflationDecayAmm
                );
            } else {
                currentInflationAmountKeeper =
                    initialAnnualInflationRateKeeper /
                    _INFLATION_DECAY_PERIOD;

                currentInflationAmountAmm = initialAnnualInflationRateAmm / _INFLATION_DECAY_PERIOD;
                initialPeriodEnded = true;
            }
            currentTotalInflation =
                currentInflationAmountLp +
                currentInflationAmountKeeper +
                currentInflationAmountAmm;
            controller.inflationManager().checkpointAllGauges();
            lastInflationDecay = block.timestamp;
        }
        return true;
    }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L50-L51

    // Used for final safety check to ensure inflation is not exceeded
    uint256 public totalAvailableToNow;

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L217-L227

    function _mint(address beneficiary, uint256 amount) internal returns (bool) {
        totalAvailableToNow += ((block.timestamp - lastEvent) * currentTotalInflation);
        uint256 newTotalMintedToNow = totalMintedToNow + amount;
        require(newTotalMintedToNow <= totalAvailableToNow, "Mintable amount exceeded");
        totalMintedToNow = newTotalMintedToNow;
        lastEvent = block.timestamp;
        token.mint(beneficiary, amount);
        _executeInflationRateUpdate();
        emit TokensMinted(beneficiary, amount);
        return true;
    }

Recommendation

Consider initializing lastEvent, lastInflationDecay in constructor().

or

Consider adding require(lastEvent != 0 && lastInflationDecay != 0, "...") to executeInflationRateUpdate().

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@danhper danhper added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 6, 2022
@danhper danhper closed this as completed Jun 7, 2022
@GalloDaSballo
Copy link
Collaborator

The warden has shown how startInflation can be bypassed, breaking the Access Control as well as the Sponsor Goal for Emissions.

Because of this I believe High Severity to be appropriate.

The sponsor has mitigated in a subsequent PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 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

4 participants