sherlock-audit / 2024-06-allora-judging

0 stars 0 forks source link

volodya - Treasury cap restriction will not hold and one block per month will be compromised #40

Open sherlock-admin3 opened 4 months ago

sherlock-admin3 commented 4 months ago

volodya

Medium

Treasury cap restriction will not hold and one block per month will be compromised

Summary

Treasury cap restriction will not hold and one block per month will be compromised

Vulnerability Detail

Once a month emissions are calculated and there is no cap checking before .AddEcosystemTokensMinted.

    if blockEmission.GT(ecosystemBalance) {
        // mint the amount of tokens required to pay out the emissions
        tokensToMint := blockEmission.Sub(ecosystemBalance)
        coins := sdk.NewCoins(sdk.NewCoin(params.MintDenom, tokensToMint))
        err = k.MintCoins(sdkCtx, coins)
        if err != nil {
            return err
        }
        err = k.MoveCoinsFromMintToEcosystem(sdkCtx, coins)
        if err != nil {
            return err
        }
        // then increment the recorded history of the amount of tokens minted
        err = k.AddEcosystemTokensMinted(ctx, tokensToMint) //@audit cap is not checked
        if err != nil {
            return err
        }
    }

x/mint/module/abci.go#L178 if minted is more than cap there would be negative number here

func GetEcosystemMintSupplyRemaining(
    ctx sdk.Context,
    k keeper.Keeper,
    params types.Params,
) (math.Int, error) {
    // calculate how many tokens left the ecosystem account is allowed to mint
    ecosystemTokensAlreadyMinted, err := k.EcosystemTokensMinted.Get(ctx)
    if err != nil {
        return math.Int{}, err
    }
    // check that you are allowed to mint more tokens and we haven't hit the max supply
    ecosystemMaxSupply := math.LegacyNewDecFromInt(params.MaxSupply).
        Mul(params.EcosystemTreasuryPercentOfTotalSupply).TruncateInt()
    return ecosystemMaxSupply.Sub(ecosystemTokensAlreadyMinted), nil
}

x/mint/module/abci.go#L89 Which will trigger error invocation here

    ratioCirculating := circulatingSupply.ToLegacyDec().Quo(maxSupply.ToLegacyDec())
    ratioEcosystemToStaked := ecosystemMintableRemaining.ToLegacyDec().Quo(networkStaked.ToLegacyDec())
    ret := fEmission.
        Mul(ratioEcosystemToStaked).
        Mul(ratioCirculating)
    if ret.IsNegative() {
        return math.LegacyDec{}, errors.Wrapf(
            types.ErrNegativeTargetEmissionPerToken,
            "target emission per token is negative: %s | %s | %s",
            ratioCirculating.String(),
            ratioEcosystemToStaked.String(),
            ret.String(),
        )
    }

    return ret, nil
}

mint/keeper/emissions.go#L148 which will compromised block

Impact

Cap restriction will not hold and one block per month will be compromised

Code Snippet

Tool used

Manual Review

Recommendation

    if blockEmission.GT(ecosystemBalance) {
        // mint the amount of tokens required to pay out the emissions
        tokensToMint := blockEmission.Sub(ecosystemBalance)
+       if( tokensToMint > ecosystemMintSupplyRemaining  ){
+                 tokensToMint := ecosystemMintSupplyRemaining
+                }
        coins := sdk.NewCoins(sdk.NewCoin(params.MintDenom, tokensToMint))
        err = k.MintCoins(sdkCtx, coins)
        if err != nil {
            return err
        }
        err = k.MoveCoinsFromMintToEcosystem(sdkCtx, coins)
        if err != nil {
            return err
        }
        // then increment the recorded history of the amount of tokens minted
        err = k.AddEcosystemTokensMinted(ctx, tokensToMint)
        if err != nil {
            return err
        }
    }
sherlock-admin2 commented 4 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/allora-network/allora-chain/pull/457

0xVolodya commented 3 months ago

Escalate I believe this is HIGH due to the fact that this function located inside BeginBlocker and block will be comprised once it will hit minting cap

sherlock-admin3 commented 3 months ago

Escalate I believe this is HIGH due to the fact that this function located inside BeginBlocker and block will be comprised once it will hit minting cap

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

WangSecurity commented 3 months ago

As I understand, there are no limitations on this issue except hitting the minting cap, and it will happen every time, but I don't see how the report shows the loss of funds and the only impact is the compromised block, no?

0xVolodya commented 3 months ago

yes, the only impact is the compromised block, medium, my bad

WangSecurity commented 3 months ago

Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 3 months ago

Result: Medium Unique

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: