Elys Modules - Elys Network

The function CalcMinCollateral in the keeper package is designed to calculate the minimum collateral required to open a position based on the provided leverage. However, the current implementation does not correctly account for the decimal precision of the collateral amount. The calculation returns the result without considering the collateral_decimals, leading to potential inaccuracies in the minimum collateral required, especially in systems where collateral assets have significant decimal precision.

// CalcMinCollateral calculates the minimum collateral required to open a position
func (k Keeper) CalcMinCollateral(ctx sdk.Context, leverage sdk.Dec) (math.Int, error) {
	// leverage must be greater than 1
	if leverage.LTE(sdk.NewDec(1)) {
		return sdk.ZeroInt(), errorsmod.Wrapf(types.ErrInvalidLeverage, "leverage must be greater than 1")
	}

	// get min borrow rate
	borrowInterestRateMin := k.GetBorrowInterestRateMin(ctx)

	// min_collateral = ( 1 / (( leverage - 1 ) * borrow_interest_rate_min )) / 10 ^ collateral_decimals
	minCollateral := sdk.NewDec(1).Quo(
		leverage.Sub(sdk.NewDec(1)).Mul(borrowInterestRateMin),
	)

	return minCollateral.TruncateInt(), nil
}

Users may be required to provide more or less collateral than accurately needed due to decimal precision not being considered. This can lead to inefficiencies, where users either over-collateralize (locking up more funds than necessary) or under-collateralize (posing a risk to the system's financial stability). Modify the CalcMinCollateral function to account for collateral_decimals. This involves adjusting the final collateral amount by the decimal precision of the collateral asset, ensuring the calculation reflects the actual minimum required collateral in terms of the asset's smallest unit.

Remediation Plan

SOLVED: The Elys Network team solved this issue by adjusting decimals.

Upon review of the CreatePool method within the Keeper module, a notable gap has been identified: the method does not implement a creation fee for new pools. This absence can potentially lead to spamming attacks, as malicious actors may exploit it to create numerous pools with minimal cost, cluttering the system and possibly degrading its performance. The lack of a creation fee for pools could lead to an influx of low-quality or spam pools being created on the platform.

func (k Keeper) CreatePool(ctx sdk.Context, msg *types.MsgCreatePool) (uint64, error) {
	sender := msg.GetSigners()[0]

	entry, found := k.assetProfileKeeper.GetEntry(ctx, ptypes.BaseCurrency)
	if !found {
		return 0, errorsmod.Wrapf(assetprofiletypes.ErrAssetProfileNotFound, "asset %s not found", ptypes.BaseCurrency)
	}
	baseCurrency := entry.Denom

	// If the fee denom is empty, set it to the base currency
	if msg.PoolParams.FeeDenom == "" {
		msg.PoolParams.FeeDenom = baseCurrency
	}

	// Get the next pool ID and increment the pool ID counter
	// Create the pool with the given pool ID
	poolId := k.GetNextPoolId(ctx)
	pool, err := types.NewBalancerPool(poolId, *msg.PoolParams, msg.PoolAssets, ctx.BlockTime())
	if err != nil {
		return 0, err
	}

	if err := pool.Validate(poolId); err != nil {
		return 0, err
	}

	address, err := sdk.AccAddressFromBech32(pool.GetAddress())
	if err != nil {
		return 0, fmt.Errorf("invalid pool address %s", pool.GetAddress())
	}

	// create and save the pool's module account to the account keeper
	if err := utils.CreateModuleAccount(ctx, k.accountKeeper, address); err != nil {
		return 0, fmt.Errorf("creating pool module account for id %d: %w", poolId, err)
	}

	// Run the initialization logic.
	if err := k.InitializePool(ctx, &pool, sender); err != nil {
		return 0, err
	}

	// Send initial liquidity to the pool's address.
	initialPoolLiquidity := msg.InitialLiquidity()
	err = k.bankKeeper.SendCoins(ctx, sender, address, initialPoolLiquidity)
	if err != nil {
		return 0, err
	}

	// Increase liquidty amount
	for _, asset := range msg.PoolAssets {
		k.RecordTotalLiquidityIncrease(ctx, sdk.Coins{asset.Token})
	}

	// emitCreatePoolEvents(ctx, poolId, msg)
	return pool.GetPoolId(), nil
}

func (suite *KeeperTestSuite) TestMsgServerCreatePool() {
	for _, tc := range []struct {
		desc              string
		senderInitBalance sdk.Coins
		poolParams        types.PoolParams
		poolAssets        []types.PoolAsset
		expSenderBalance  sdk.Coins
		expTotalLiquidity sdk.Coins
		expLpCommitment   sdk.Coin
		expPass           bool
	}{
		{
			desc:              "zero tvl pool creation",
			senderInitBalance: sdk.Coins{sdk.NewInt64Coin(ptypes.Eden, 1000000), sdk.NewInt64Coin(ptypes.Elys, 1000000)},
			poolParams: types.PoolParams{
				SwapFee:                     sdk.ZeroDec(),
				ExitFee:                     sdk.ZeroDec(),
				UseOracle:                   false,
				WeightBreakingFeeMultiplier: sdk.ZeroDec(),
				WeightBreakingFeeExponent:   sdk.NewDecWithPrec(25, 1), // 2.5
				ExternalLiquidityRatio:      sdk.NewDec(1),
				WeightRecoveryFeePortion:    sdk.NewDecWithPrec(10, 2), // 10%
				ThresholdWeightDifference:   sdk.ZeroDec(),
				FeeDenom:                    ptypes.BaseCurrency,
			},
			poolAssets: []types.PoolAsset{
				{
					Token:  sdk.NewInt64Coin(ptypes.Eden, 1000000),
					Weight: sdk.OneInt(),
				},
				{
					Token:  sdk.NewInt64Coin(ptypes.Elys, 1000000),
					Weight: sdk.OneInt(),
				},
			},
			expSenderBalance: sdk.Coins{},
			expLpCommitment:  sdk.NewCoin("amm/pool/1", sdk.NewInt(100).Mul(types.OneShare)),
			expPass:          true,
		},
		{
			desc:              "positive tvl pool creation",
			senderInitBalance: sdk.Coins{sdk.NewInt64Coin(ptypes.Eden, 1000000), sdk.NewInt64Coin(ptypes.BaseCurrency, 1000000)},
			poolParams: types.PoolParams{
				SwapFee:                     sdk.ZeroDec(),
				ExitFee:                     sdk.ZeroDec(),
				UseOracle:                   false,
				WeightBreakingFeeMultiplier: sdk.ZeroDec(),
				WeightBreakingFeeExponent:   sdk.NewDecWithPrec(25, 1), // 2.5
				ExternalLiquidityRatio:      sdk.NewDec(1),
				WeightRecoveryFeePortion:    sdk.NewDecWithPrec(10, 2), // 10%
				ThresholdWeightDifference:   sdk.ZeroDec(),
				FeeDenom:                    ptypes.BaseCurrency,
			},
			poolAssets: []types.PoolAsset{
				{
					Token:  sdk.NewInt64Coin(ptypes.Eden, 1000000),
					Weight: sdk.OneInt(),
				},
				{
					Token:  sdk.NewInt64Coin(ptypes.BaseCurrency, 1000000),
					Weight: sdk.OneInt(),
				},
			},
			expSenderBalance: sdk.Coins{},
			expLpCommitment:  sdk.NewCoin("amm/pool/1", sdk.NewInt(2).Mul(types.OneShare)),
			expPass:          true,
		},
		{
			desc:              "not enough balance to create pool",
			senderInitBalance: sdk.Coins{sdk.NewInt64Coin(ptypes.Eden, 1000000)},
			poolParams: types.PoolParams{
				SwapFee:                     sdk.ZeroDec(),
				ExitFee:                     sdk.ZeroDec(),
				UseOracle:                   false,
				WeightBreakingFeeMultiplier: sdk.ZeroDec(),
				WeightBreakingFeeExponent:   sdk.NewDecWithPrec(25, 1), // 2.5
				ExternalLiquidityRatio:      sdk.NewDec(1),
				WeightRecoveryFeePortion:    sdk.NewDecWithPrec(10, 2), // 10%
				ThresholdWeightDifference:   sdk.ZeroDec(),
				FeeDenom:                    ptypes.BaseCurrency,
			},
			poolAssets: []types.PoolAsset{
				{
					Token:  sdk.NewInt64Coin(ptypes.Eden, 1000000),
					Weight: sdk.OneInt(),
				},
				{
					Token:  sdk.NewInt64Coin(ptypes.BaseCurrency, 1000000),
					Weight: sdk.OneInt(),
				},
			},
			expSenderBalance: sdk.Coins{},
			expLpCommitment:  sdk.Coin{},
			expPass:          false,
		},
	} {
		suite.Run(tc.desc, func() {
			suite.SetupTest()
			suite.SetupStableCoinPrices()

			// bootstrap accounts
			sender := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address())

			// bootstrap balances
			err := suite.app.BankKeeper.MintCoins(suite.ctx, minttypes.ModuleName, tc.senderInitBalance)
			suite.Require().NoError(err)
			err = suite.app.BankKeeper.SendCoinsFromModuleToAccount(suite.ctx, minttypes.ModuleName, sender, tc.senderInitBalance)
			suite.Require().NoError(err)

			// execute function
			msgServer := keeper.NewMsgServerImpl(suite.app.AmmKeeper)
			resp, err := msgServer.CreatePool(
				sdk.WrapSDKContext(suite.ctx),
				&types.MsgCreatePool{
					Sender:     sender.String(),
					PoolParams: &tc.poolParams,
					PoolAssets: tc.poolAssets,
				})
			if !tc.expPass {
				suite.Require().Error(err)
			} else {
				suite.Require().NoError(err)
				suite.Require().Equal(resp.PoolID, uint64(1))

				pools := suite.app.AmmKeeper.GetAllPool(suite.ctx)
				suite.Require().Len(pools, 1)
				suite.Require().Equal(pools[0].PoolId, uint64(1))
				suite.Require().Equal(pools[0].PoolParams, tc.poolParams)
				suite.Require().Equal(pools[0].TotalShares.Amount.String(), tc.expLpCommitment.Amount.String())

				totalWeight := sdk.ZeroInt()
				for _, poolAsset := range tc.poolAssets {
					totalWeight = totalWeight.Add(poolAsset.Weight)
				}
				suite.Require().Equal(pools[0].TotalWeight.String(), totalWeight.MulRaw(types.GuaranteedWeightPrecision).String())

				// check balance change on sender
				balances := suite.app.BankKeeper.GetAllBalances(suite.ctx, sender)
				suite.Require().Equal(balances.String(), tc.expSenderBalance.String())

				// check lp token commitment
				commitments := suite.app.CommitmentKeeper.GetCommitments(suite.ctx, sender.String())
				suite.Require().Len(commitments.CommittedTokens, 1)
				suite.Require().Equal(commitments.CommittedTokens[0].Denom, tc.expLpCommitment.Denom)
				suite.Require().Equal(commitments.CommittedTokens[0].Amount.String(), tc.expLpCommitment.Amount.String())
			}
		})
	}
}

Introduce a fee for creating new pools. This fee should be set at a level that discourages spamming while remaining reasonable for legitimate users.

Remediation Plan

SOLVED: The Elys Network team solved this issue by adding a pool creation fee.

Inadequate handling of large WebAssembly (WASM) files on the chain can lead to performance issues, slow startup times, and potentially cause system failure. It can cause to DOS.

echo "Running store code"
RES=$(client tx wasm store artifacts/larger_wasm_file.wasm --from alice -y --output json -b block $TXFLAG) 
CODE_ID=$(echo $RES | jq -r '.logs[0].events[-1].attributes[0].value') 
echo "Got CODE_ID = $CODE_ID"

echo "Deploying contract"
# swallow output
OUT=$(client tx wasm instantiate $CODE_ID "$INIT" --from alice --label "my first contract" --gas-prices 10$FEE_DENOM --gas auto --gas-adjustment 1.3 -b block -y --no-admin $NODE --chain-id $CHAIN_ID)
ADDR=$(client query wasm list-contract-by-code $CODE_ID --output json $NODE | jq -r '.contracts[0]')
echo "Could not deploy larger wasm file"

To prevent issues related to inadequate handling of large WASM files, increase or override the default max file size limit to allow the system to handle larger WASM files. (https://github.com/osmosis-labs/osmosis/blob/574233334f7b69fe1a58d2f79ef189dd68d32eee/app/app.go#L183)

Remediation Plan

SOLVED: The Elys Network team solved the issue.

In the provided UpdateEntry function, part of a system managing entries in IBC. The function updates an entry with various fields, including IbcChannelId and IbcCounterpartyChannelId. However, it lacks verification for the correctness and validity of these Inter-Blockchain Communication (IBC) channel identifiers.

func (k msgServer) UpdateEntry(goCtx context.Context, msg *types.MsgUpdateEntry) (*types.MsgUpdateEntryResponse, error) {
	if k.authority != msg.Authority {
		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority)
	}

	ctx := sdk.UnwrapSDKContext(goCtx)

	// Check if the value exists
	entry, isFound := k.GetEntry(ctx, msg.BaseDenom)
	if !isFound {
		return nil, errorsmod.Wrap(sdkerrors.ErrKeyNotFound, "entry not set")
	}

	// Checks if the the msg authority is the same as the current owner
	if msg.Authority != entry.Authority {
		return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "incorrect owner")
	}

	entry = types.Entry{
		Authority:                msg.Authority,
		BaseDenom:                msg.BaseDenom,
		Decimals:                 msg.Decimals,
		Denom:                    msg.Denom,
		Path:                     msg.Path,
		IbcChannelId:             msg.IbcChannelId,
		IbcCounterpartyChannelId: msg.IbcCounterpartyChannelId,
		DisplayName:              msg.DisplayName,
		DisplaySymbol:            msg.DisplaySymbol,
		Network:                  msg.Network,
		Address:                  msg.Address,
		ExternalSymbol:           msg.ExternalSymbol,
		TransferLimit:            msg.TransferLimit,
		Permissions:              msg.Permissions,
		UnitDenom:                msg.UnitDenom,
		IbcCounterpartyDenom:     msg.IbcCounterpartyDenom,
		IbcCounterpartyChainId:   msg.IbcCounterpartyChainId,
		CommitEnabled:            msg.CommitEnabled,
		WithdrawEnabled:          msg.WithdrawEnabled,
	}

	k.SetEntry(ctx, entry)

	return &types.MsgUpdateEntryResponse{}, nil
}

Consider validating IBC channel.

Remediation Plan

SOLVED: The Elys Network team solved the issue.

The MsgUpdateBrokerAddress function in the given code is designed to update the broker address in app chain. This function creates an instance of MsgUpdateBrokerAddress with a specified creator and broker address. However, a crucial issue is identified in the associated ValidateBasic method: it does not validate the BrokerAddress. Currently, the ValidateBasic method only validates the Creator field, overlooking the validation of BrokerAddress. For instance, some swap operations can fail.

Enhance the ValidateBasic function to include validation for the BrokerAddress. This validation should ensure that the BrokerAddress is correctly formatted and adheres to the expected address standards.

Remediation Plan

SOLVED: The Elys Network team solved the issue.

The current implementation of Elys Network Keeper methods (GetLatestPriceFromAssetAndSource and GetLatestPriceFromAnySource) retrieves the latest price of an asset, either from a specific source or any source. While this provides the most recent price data, it may not accurately reflect the market price due to short-term price fluctuations or potential manipulation. To enhance the reliability of the pricing mechanism, we propose the implementation of Time-Weighted Average Price (TWAP).

func (k msgServer) FeedPrice(goCtx context.Context, msg *types.MsgFeedPrice) (*types.MsgFeedPriceResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

	feeder, found := k.Keeper.GetPriceFeeder(ctx, msg.Provider)
	if !found {
		return nil, types.ErrNotAPriceFeeder
	}

	if !feeder.IsActive {
		return nil, types.ErrPriceFeederNotActive
	}

	price := types.Price{
		Provider:  msg.Provider,
		Asset:     msg.Asset,
		Price:     msg.Price,
		Source:    msg.Source,
		Timestamp: uint64(ctx.BlockTime().Unix()),
	}

	k.SetPrice(ctx, price)
	return &types.MsgFeedPriceResponse{}, nil
}

Modify the existing GetPrice function to calculate the TWAP. This might involve fetching multiple price points within a defined time window and computing their average.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

In the current implementation of JoinPoolNoSwap, the implementation does not consider with diff in the calculations of the expected shares out and calculated shares out. The diff in calculations is at most 1 denom-coin - rounding error. There will be dust.

func (k Keeper) JoinPoolNoSwap(
	ctx sdk.Context,
	sender sdk.AccAddress,
	poolId uint64,
	shareOutAmount sdk.Int,
	tokenInMaxs sdk.Coins,
) (tokenIn sdk.Coins, sharesOut sdk.Int, err error) {
	// defer to catch panics, in case something internal overflows.
	defer func() {
		if r := recover(); r != nil {
			tokenIn = sdk.Coins{}
			sharesOut = sdk.Int{}
			err = fmt.Errorf("function JoinPoolNoSwap failed due to internal reason: %v", r)
		}
	}()
	// all pools handled within this method are pointer references, `JoinPool` directly updates the pools
	pool, poolExists := k.GetPool(ctx, poolId)
	if !poolExists {
		return nil, sdk.ZeroInt(), types.ErrInvalidPoolId
	}

	if !pool.PoolParams.UseOracle {
		tokensIn := tokenInMaxs
		if len(tokensIn) != 1 {
			// we do an abstract calculation on the lp liquidity coins needed to have
			// the designated amount of given shares of the pool without performing swap
			neededLpLiquidity, err := pool.GetMaximalNoSwapLPAmount(shareOutAmount)
			if err != nil {
				return nil, sdk.ZeroInt(), err
			}

			// check that needed lp liquidity does not exceed the given `tokenInMaxs` parameter. Return error if so.
			// if tokenInMaxs == 0, don't do this check.
			if tokenInMaxs.Len() != 0 {
				if !(neededLpLiquidity.DenomsSubsetOf(tokenInMaxs)) {
					return nil, sdk.ZeroInt(), errorsmod.Wrapf(types.ErrLimitMaxAmount, "TokenInMaxs does not include all the tokens that are part of the target pool,"+
						" upperbound: %v, needed %v", tokenInMaxs, neededLpLiquidity)
				} else if !(tokenInMaxs.DenomsSubsetOf(neededLpLiquidity)) {
					return nil, sdk.ZeroInt(), errorsmod.Wrapf(types.ErrDenomNotFoundInPool, "TokenInMaxs includes tokens that are not part of the target pool,"+
						" input tokens: %v, pool tokens %v", tokenInMaxs, neededLpLiquidity)
				}
				if !(tokenInMaxs.IsAllGTE(neededLpLiquidity)) {
					return nil, sdk.ZeroInt(), errorsmod.Wrapf(types.ErrLimitMaxAmount, "TokenInMaxs is less than the needed LP liquidity to this JoinPoolNoSwap,"+
						" upperbound: %v, needed %v", tokenInMaxs, neededLpLiquidity)
				}
			}

			tokensIn = neededLpLiquidity
		}

		sharesOut, err = pool.JoinPool(ctx, k.oracleKeeper, k.accountedPoolKeeper, tokensIn)
		if err != nil {
			return nil, sdk.ZeroInt(), err
		}

		// sanity check, don't return error as not worth halting the LP. We know its not too much.
		if sharesOut.LT(shareOutAmount) {
			ctx.Logger().Error(fmt.Sprintf("Expected to JoinPoolNoSwap >= %s shares, actually did %s shares",
				shareOutAmount, sharesOut))
		}

		err = k.applyJoinPoolStateChange(ctx, pool, sender, sharesOut, tokensIn)

		// Increase liquidty amount
		k.RecordTotalLiquidityIncrease(ctx, tokensIn)

		return tokensIn, sharesOut, err
	}

	// on oracle pool, full tokenInMaxs are used regardless shareOutAmount
	sharesOut, err = pool.JoinPool(ctx, k.oracleKeeper, k.accountedPoolKeeper, tokenInMaxs)
	if err != nil {
		return nil, sdk.ZeroInt(), err
	}

	// sanity check, don't return error as not worth halting the LP. We know its not too much.
	if sharesOut.LT(shareOutAmount) {
		ctx.Logger().Error(fmt.Sprintf("Expected to JoinPoolNoSwap >= %s shares, actually did %s shares",
			shareOutAmount, sharesOut))
	}

	err = k.applyJoinPoolStateChange(ctx, pool, sender, sharesOut, tokenInMaxs)

	// Increase liquidty amount
	k.RecordTotalLiquidityIncrease(ctx, tokenInMaxs)

	return tokenInMaxs, sharesOut, err
}

Consider adding a difference check with the bounds.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

For blockchains that support custom token transfer logic, such as blacklists for specific tokens like USDC, it is crucial to ensure that all token transfers in the Begin/EndBlock stages properly handle and catch potential panics. While this is generally a straightforward task, it is commonly overlooked in one specific context: bulk coin sends.

The Cosmos SDK allows for multiple coins to be transferred in a single function call through its SendCoins function. This function acts as a black box, which means it does not provide the ability to validate each individual token transfer. As a result, developers often overlook the necessity to handle panics within this function.

If a single panic is triggered during a call to SendCoins in the Begin/EndBlock stages, it can lead to a complete halt of the blockchain. This highlights the importance of properly handling panics in bulk coin send operations, as failing to do so can have severe consequences on the chain's availability and functionality.

Therefore, when implementing custom token transfer logic or working with bulk coin sends, it is essential to thoroughly review the code and ensure that all potential panic scenarios are properly caught and handled. This proactive approach can help prevent unexpected chain halts and maintain the stability and reliability of the blockchain.

Example Code Location : https://github.com/elys-network/elys/blob/6a6ec2162613ba8bf7d683162698516e7c6eb7ba/x/perpetual/keeper/handle_funding_fee_distribution.go#L80-L81

While one can catch the panic on the entire SendCoins call, this would mean that an attacker can DoS all transfers in the batch. Thus, the solution for these situations is to transfer coins one by one with SendCoin and verify each transfer so that problematic ones can be skipped.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

The spec file intends to outline the common structure for specifications within this directory. The assetprofile - parameter - commitment - stablestake - oracle - accountedpool - amm modules are missing spec. This documentation is segmented into developer-focused messages and end-user-facing messages. These messages may be shown to the end user (the human) at the time that they will interact with the module.

It is recommended that modules are fully annotated using spec for all available functionalities.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

The current implementation of the FeedPrice method in the msgServer struct lacks an essential security feature: asset whitelisting for the price feeder. This function is designed to update the price of an asset provided by a price feeder, but it does not verify whether the asset being updated is recognized or authorized by the system.

func (k msgServer) FeedPrice(goCtx context.Context, msg *types.MsgFeedPrice) (*types.MsgFeedPriceResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

	feeder, found := k.Keeper.GetPriceFeeder(ctx, msg.Provider)
	if !found {
		return nil, types.ErrNotAPriceFeeder
	}

	if !feeder.IsActive {
		return nil, types.ErrPriceFeederNotActive
	}

	price := types.Price{
		Provider:  msg.Provider,
		Asset:     msg.Asset,
		Price:     msg.Price,
		Source:    msg.Source,
		Timestamp: uint64(ctx.BlockTime().Unix()),
	}

	k.SetPrice(ctx, price)
	return &types.MsgFeedPriceResponse{}, nil
}

Introduce a mechanism to maintain a whitelist of approved assets. The FeedPrice method should include a check to ensure that the asset provided in msg.Asset is part of this whitelist.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

The functions NewMsgCreateEntry, NewMsgUpdateEntry, and NewMsgDeleteEntry are part of a system that manages entries. However, an issue arises in their ValidateBasic methods: there is a lack of comprehensive validation for all the fields in these messages. Currently, the validation only checks the Authority field's format, neglecting other crucial fields like BaseDenom, Denom, Path, IbcChannelId, and others. This insufficient validation can lead to potential operational failures or vulnerabilities, as malformed or invalid data might be processed by the system.

func NewMsgUpdateEntry(
    authority string,
    baseDenom string,
    decimals uint64,
    denom string,
    path string,
    ibcChannelId string,
    ibcCounterpartyChannelId string,
    displayName string,
    displaySymbol string,
    network string,
    address string,
    externalSymbol string,
    transferLimit string,
    permissions []string,
    unitDenom string,
    ibcCounterpartyDenom string,
    ibcCounterpartyChainId string,
) *MsgUpdateEntry {
    return &MsgUpdateEntry{
        Authority:                authority,
        BaseDenom:                baseDenom,
        Decimals:                 decimals,
        Denom:                    denom,
        Path:                     path,
        IbcChannelId:             ibcChannelId,
        IbcCounterpartyChannelId: ibcCounterpartyChannelId,
        DisplayName:              displayName,
        DisplaySymbol:            displaySymbol,
        Network:                  network,
        Address:                  address,
        ExternalSymbol:           externalSymbol,
        TransferLimit:            transferLimit,
        Permissions:              permissions,
        UnitDenom:                unitDenom,
        IbcCounterpartyDenom:     ibcCounterpartyDenom,
        IbcCounterpartyChainId:   ibcCounterpartyChainId,
    }
}

Extend the ValidateBasic method in each message type to include validation checks for all relevant fields. These checks should ensure that the fields are correctly formatted and meet the expected standards.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

Elys Network, which utilizes the Cosmos SDK version 0.47.4, is exposed to multiple vulnerabilities identified within the specified version of the SDK. The issues encompass a range of security flaws that could potentially impact the stability, security, and integrity of the Elys Network. The vulnerabilities are documented in the following GitHub advisories:

• GHSA-4j93-fm92-rp4m: Describes a critical flaw that could allow for unintended behavior or access.

• GHSA-2557-x9mg-76w8: Details vulnerabilities that may lead to denial of service (DoS) attacks or affect the network's operational integrity.

• GHSA-95rx-m9m5-m94v: Outlines issues that could potentially be exploited to compromise the confidentiality, integrity, or availability of the network.

• GHSA-86h5-xcpx-cfqc: Highlights flaws that might allow malicious actors to impact the network adversely.

Promptly upgrade to the latest version of the Cosmos SDK that addresses these vulnerabilities. Ensure thorough testing of the upgraded version in a test environment before deploying it to the mainnet to confirm compatibility and stability.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

In the current implementation of the FeedPrice function, the timestamp for the price feed is derived from the block time by converting it to Unix time. However, relying solely on the block time may not be sufficient in certain scenarios, as it can potentially lead to issues such as:

1. Time Manipulation Attacks: If an attacker can manipulate the block time, they may be able to feed incorrect prices by adjusting the timestamp.

2. Timestamp Collisions: If multiple price feeds are submitted within the same Unix timestamp, there could be potential collisions or overwriting of prices, leading to data loss or inconsistencies.

3. Lack of Determinism: Using only the block time introduces a non-deterministic element, as the block time can vary slightly between nodes due to network latency or other factors.

To address these potential issues and improve the security and reliability of the price feeding mechanism, it is recommended to utilize both the block time and the block height in the price feed message.

func (k msgServer) FeedPrice(goCtx context.Context, msg *types.MsgFeedPrice) (*types.MsgFeedPriceResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

	feeder, found := k.Keeper.GetPriceFeeder(ctx, msg.Provider)
	if !found {
		return nil, types.ErrNotAPriceFeeder
	}

	if !feeder.IsActive {
		return nil, types.ErrPriceFeederNotActive
	}

	price := types.Price{
		Provider:  msg.Provider,
		Asset:     msg.Asset,
		Price:     msg.Price,
		Source:    msg.Source,
		Timestamp: uint64(ctx.BlockTime().Unix()),
	}

	k.SetPrice(ctx, price)
	return &types.MsgFeedPriceResponse{}, nil
}

Modify the MsgFeedPrice message to include the block height in addition to the timestamp.

Remediation Plan

SOLVED: The Elys Network team solved the issue by implementing the suggested recommendation.

The Elys project uses a function called PowApprox to calculate approximate values for the exponentiation operation (a^b). This function is implemented using a Taylor series approximation, and it uses a while loop to iterate until the terms being added are sufficiently small. However, the implementation does not have a bound on the maximum number of iterations, which can potentially lead to a Denial of Service (DoS) vulnerability.

The issue arises when specific values of a and b are chosen, such as a = 1.999999999999999 and b = 0.1. In these cases, the PowApprox function can take an excessive number of iterations (over two million iterations on an M1 processor) to converge, resulting in a prolonged computation time of over 800 milliseconds.

This long runtime is not accounted for in the gas costs of transactions that use the PowApprox function. As a result, an attacker can craft a transaction that calls PowApprox with carefully chosen values, causing the Elys nodes to spend an inordinate amount of time computing this function without having to pay a proportional gas cost.

// Contract: 0 < base <= 2
// 0 <= exp < 1.
func PowApprox(base sdk.Dec, exp sdk.Dec, precision sdk.Dec) sdk.Dec {
	if !base.IsPositive() {
		panic(fmt.Errorf("base must be greater than 0"))
	}

	if exp.IsZero() {
		return sdk.OneDec()
	}

	// Common case optimization
	// Optimize for it being equal to one-half
	if exp.Equal(one_half) {
		output, err := base.ApproxSqrt()
		if err != nil {
			panic(err)
		}
		return output
	}
	// TODO: Make an approx-equal function, and then check if exp * 3 = 1, and do a check accordingly

	// We compute this via taking the maclaurin series of (1 + x)^a
	// where x = base - 1.
	// The maclaurin series of (1 + x)^a = sum_{k=0}^{infty} binom(a, k) x^k
	// Binom(a, k) takes the natural continuation on the first parameter, namely that
	// Binom(a, k) = N/D, where D = k!, and N = a(a-1)(a-2)...(a-k+1)
	// Next we show that the absolute value of each term is less than the last term.
	// Note that the change in term n's value vs term n + 1 is a multiplicative factor of
	// v_n = x(a - n) / (n+1)
	// So if |v_n| < 1, we know that each term has a lesser impact on the result than the last.
	// For our bounds on |x| < 1, |a| < 1,
	// it suffices to see for what n is |v_n| < 1,
	// in the worst parameterization of x = 1, a = -1.
	// v_n = |(-1 + epsilon - n) / (n+1)|
	// So |v_n| is always less than 1, as n ranges over the integers.
	//
	// Note that term_n of the expansion is 1 * prod_{i=0}^{n-1} v_i
	// The error if we stop the expansion at term_n is:
	// error_n = sum_{k=n+1}^{infty} term_k
	// At this point we further restrict a >= 0, so 0 <= a < 1.
	// Now we take the _INCORRECT_ assumption that if term_n < p, then
	// error_n < p.
	// This assumption is obviously wrong.
	// However our usages of this function don't use the full domain.
	// With a > 0, |x| << 1, and p sufficiently low, perhaps this actually is true.

	// TODO: Check with our parameterization
	// TODO: If theres a bug, balancer is also wrong here :thonk:

	base = base.Clone()
	x, xneg := AbsDifferenceWithSign(base, one)
	term := sdk.OneDec()
	sum := sdk.OneDec()
	negative := false

	a := exp.Clone()
	bigK := sdk.NewDec(0)
	// TODO: Document this computation via taylor expansion
	for i := int64(1); term.GTE(precision); i++ {
		// At each iteration, we need two values, i and i-1.
		// To avoid expensive big.Int allocation, we reuse bigK variable.
		// On this line, bigK == i-1.
		c, cneg := AbsDifferenceWithSign(a, bigK)
		// On this line, bigK == i.
		bigK.Set(sdk.NewDec(i)) // TODO: O(n) bigint allocation happens
		term.MulMut(c).MulMut(x).QuoMut(bigK)

		// a is mutated on absDifferenceWithSign, reset
		a.Set(exp)

		if term.IsZero() {
			break
		}
		if xneg {
			negative = !negative
		}

		if cneg {
			negative = !negative
		}

		if negative {
			sum.SubMut(term)
		} else {
			sum.AddMut(term)
		}
	}
	return sum
}

// calculate a^b
// assumption: a is between 0 and 2, b is between 0 and 1
fn PowApprox(a,b) {
  total <- 1
  i <- 0
  term <- 1
  const precision = 0.00000001
  // (the real implementation took precision as a function parameter rather than a constant)
  while abs(term) >= precision {
    i <- i + 1
    term <- term * ((b-(i-1)) / i) * (a-1)
    total <- total + term
  }
  return total
}

Implement a maximum iteration limit or a timeout mechanism in the PowApprox function to prevent excessive computation times.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

We've identified a potential issue with the precision of the initial shares allocated to the first liquidity provider (LP) of a pool. Currently, the protocol initializes a pool with 10^18 shares. However, this might not offer enough precision, especially in scenarios where the initial liquidity provider is a significant entity (like the project team) and contributes a substantial amount of liquidity. For context, Balancer, a well-known protocol in the DeFi space, initializes its pools with a higher precision, specifically 100 * 10^18 shares (reference: Balancer's BConst.sol). This higher level of precision allows for more granular control and accuracy in the distribution of pool shares, which is particularly important in the DeFi ecosystem where large and fractional values are common.

Increase the precision of initial pool shares from 10^18 to 100 * 10^18.

Remediation Plan

SOLVED: The Elys Network team solved the issue by adjusting the share amount.

All the transaction and query commands for all the modules are missing a long message to provide description about their usage, which will be helpful for users and external developers.

It is recommended to specify a long message for all transaction commands. Each command should provide a description of how to use the command correctly.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.

During periods of high network activity, maintaining liveness and usability becomes challenging without a robust fee market mechanism in place. While there are various approaches, the Ethereum Improvement Proposal (EIP-1559) has proven to be an effective default solution. However, implementing it directly in the mempool, as some blockchains have done, is suboptimal compared to integrating the fee market directly into the consensus layer.

The superior long-term solution is to leverage the ABCI 2.0 protocol and integrate the fee market into the consensus layer. This approach offers several advantages, including improved reliability, scalability, and adaptability to changing network conditions. By leveraging ABCI 2.0, blockchains can seamlessly adopt the fee market mechanism without the need for extensive modifications or hard forks.

Conduct thorough testing and simulations to ensure the fee market mechanism operates as intended under various network conditions, including periods of high usage and congestion.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.

Rate limits are safety controls that disable certain functionality of a system when a pre-defined threshold is surpassed. In this context, we categorize rate limit mechanisms that don’t require external trigger invocation, as the tripping conditions are defined in the protocol. As such, rate limit thresholds tend to be simpler and numeric. (Osmosis Implementation : https://commonwealth.im/osmosis/discussion/8543-set-ibc-rate-limits)

Implement rate-limiting on the IBC like an Osmosis.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.

In blockchains that allow for arbitrary token transfer hooks and execute them during the Begin/EndBlock stages, a potential vulnerability exists where an attacker can create a token that triggers an infinitely looping CosmWasm contract. When this malicious token is transferred during the next block's BeginBlock, it can cause the entire chain to halt due to the unintended infinite loop.

This issue highlights the importance of properly handling and limiting the execution of arbitrary code during critical phases of the blockchain's operation, such as the BeginBlocker and EndBlocker operations. Failing to impose appropriate constraints can lead to Denial of Service (DoS) attacks and severe disruptions to the chain's availability and functionality.

The root cause of this vulnerability lies in the lack of proper gas metering or execution time limits for the arbitrary code executed during token transfers. An attacker can exploit this by crafting a token that triggers an infinite loop or resource-intensive computation, effectively locking up the node's resources and preventing further block processing.

Implement gas metering or execution time limits for all arbitrary code executed during token transfers or other critical blockchain operations. This can be achieved by leveraging the existing gas metering mechanisms in the Cosmos SDK or implementing custom time-based limits.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.

In the provided code snippet, the ClearOutdatedSlippageTrack function is responsible for deleting expired slippage tracks. However, the expiration date is currently hardcoded as a constant value of 7 days (86400 * 7 seconds). This hardcoded value limits the flexibility of the system and may not be suitable for all scenarios or deployment environments.

Having a fixed expiration date for slippage tracks can lead to potential issues, such as:

1. Lack of Adaptability: Different projects or use cases may require different retention periods for slippage tracks, depending on their specific requirements or analysis needs.

2. Limited Governance: The expiration date is hardcoded in the system, preventing the community or governance mechanisms from adjusting or fine-tuning this parameter based on changing circumstances or feedback.

3. Maintenance Challenges: If the desired expiration period needs to be changed in the future, it would require modifying the codebase and potentially deploying a new version of the software, which can be a time-consuming and error-prone process.

Code Location : https://github.com/elys-network/elys/blob/6a6ec2162613ba8bf7d683162698516e7c6eb7ba/x/amm/keeper/abci.go#L174-L175

Create a new governance parameter that represents the slippage expiration date (e.g., SlippageTrackExpirationDays). This parameter should be stored in the module's parameter store and initialized with a sensible default value (e.g., 7 days).

Remediation Plan

SOLVED: The Elys Network team solved the issue by adding suggested recommendations.

In the RecordWithdrawValidatorCommission function within the keeper, there is a potential issue where the function does not return an error in case of failure.

https://github.com/elys-network/elys/blob/7a20f784857ecda997c6cc55dbd52068f13926c1/x/incentive/keeper/keeper_withdraw.go#L191-L192

	// Eden
	unclaimed := commitments.GetRewardUnclaimedForDenom(ptypes.Eden)
	if !unclaimed.IsZero() {
		err = k.cmk.RecordWithdrawValidatorCommission(ctx, delegator, validator, ptypes.Eden, unclaimed)
	}

	// EdenB
	unclaimed = commitments.GetRewardUnclaimedForDenom(ptypes.EdenB)
	if !unclaimed.IsZero() {
		err = k.cmk.RecordWithdrawValidatorCommission(ctx, delegator, validator, ptypes.EdenB, unclaimed)
	}

Modify the RecordWithdrawValidatorCommission function to handle errors gracefully and return them consistently.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.