Prepared by:
HALBORN
Last Updated 11/27/2024
Date of Engagement by: May 27th, 2024 - June 19th, 2024
100% of all REPORTED Findings have been addressed
All findings
12
Critical
3
High
2
Medium
1
Low
4
Informational
2
The Elys Network team
engaged Halborn to conduct a security assessment on their cosmos appchain modules, beginning on 05/27/2024 and ending on 06/19/2024. The security assessment was scoped to the sections of code that pertain to the staking & masterchef modules of app chain. Commit hashes and further details can be found in the Scope section of this report.
Halborn was provided 3 weeks for the engagement and assigned 1 full-time security engineer to review the security of the modules in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
- Ensure that the EStaking & MasterChef Modules operate as intended.
- Identify potential security issues with the custom modules used in the Elys Chain.
In summary, Halborn identified some security issues that were mostly addressed by the Elys Network team
.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the custom modules. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of structures and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the assessment:
- Research into architecture and purpose.
- Static Analysis of security for scoped repository, and imported functions. (e.g., staticcheck
, gosec
, unconvert
, codeql
, ineffassign
and semgrep
)
- Manual Assessment for discovering security vulnerabilities on the codebase.
- Ensuring the correctness of the codebase.
- Dynamic Analysis of files and modules related to the Elys Network Modules.
External libraries and financial-related attacks.
New features/implementations after/with the remediation commit IDs
Changes that occur outside of the scope of PRs.
EXPLOITABILITY METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
3
High
2
Medium
1
Low
4
Informational
2
Security analysis | Risk level | Remediation Date |
---|---|---|
Reward Inflation and Distribution Inconsistencies due to Improper LastUpdatedBlock Initialization | Critical | Solved - 06/18/2024 |
Chain Halt due to Panic in MustAccAddressFromBech32 | Critical | Solved - 11/27/2024 |
Potential Chain Halt Due to Blocked Protocol Revenue Address | Critical | Solved - 11/27/2024 |
Integer Overflow in AddExternalIncentive Function | High | Solved - 06/18/2024 |
Hardcoded Limit for Delegation Retrieval and Lack of Error Handling | High | Solved - 06/18/2024 |
Misleading Error Messages and Comments in msgClaimRewards and performMsgClaimRewards Functions | Medium | Solved - 06/18/2024 |
Lack of Event Emission in Message Handlers | Low | Solved - 06/18/2024 |
Lack of spec on the modules | Low | Solved - 06/18/2024 |
ASA-2024-004: Default Evidence Configuration Parameters May Limit Window of Validity for the Noble App Chain | Low | Risk Accepted |
ASA-2023-002: Default BlockParams.MaxBytes Configuration May Increase Block Times and Affect Consensus Participation in the Noble App Chain | Low | Solved - 11/27/2024 |
Missing Long Descriptions In Cli Affects Usability And User Experience | Informational | Acknowledged |
Potential Incorrect Consensus Version in the estaking Module | Informational | Acknowledged |
// Critical
The UpdateAccPerShare
function initializes the PoolRewardInfo
struct if it is not found in the store. However, the LastUpdatedBlock
field is set to 0 in this case, which could lead to several issues related to reward distribution.
Setting the LastUpdatedBlock
field to 0 when initializing the PoolRewardInfo
struct can result in the following problems:
1. Reward Inflation: When the system is first initialized or after a prolonged period of inactivity, users may receive an excessive amount of rewards. This is because the PoolAccRewardPerShare
calculation will consider the entire block history since the genesis block, leading to an inflated reward accumulation.
2. Inconsistent Reward Distribution: Users who join the system later may receive a disproportionate amount of rewards compared to those who have been participating from the beginning. This creates an unfair distribution of rewards and could potentially discourage early adopters.
3. Incorrect Reward Calculations: The improper initialization of LastUpdatedBlock
can lead to incorrect reward calculations, as the system will not have an accurate reference point for tracking the reward accumulation over time.
func (k Keeper) UpdateAccPerShare(ctx sdk.Context, poolId uint64, rewardDenom string, amount sdk.Int) {
poolRewardInfo, found := k.GetPoolRewardInfo(ctx, poolId, rewardDenom)
if !found {
poolRewardInfo = types.PoolRewardInfo{
PoolId: poolId,
RewardDenom: rewardDenom,
PoolAccRewardPerShare: sdk.NewDec(0),
LastUpdatedBlock: 0,
}
}
totalCommit := k.GetPoolTotalCommit(ctx, poolId)
if totalCommit.IsZero() {
return
}
poolRewardInfo.PoolAccRewardPerShare = poolRewardInfo.PoolAccRewardPerShare.Add(
math.LegacyNewDecFromInt(amount.Mul(ammtypes.OneShare)).
Quo(math.LegacyNewDecFromInt(totalCommit)),
)
poolRewardInfo.LastUpdatedBlock = uint64(ctx.BlockHeight())
k.SetPoolRewardInfo(ctx, poolRewardInfo)
}
func (k Keeper) UpdateUserRewardPending(ctx sdk.Context, poolId uint64, rewardDenom string, user string, isDeposit bool, amount sdk.Int) {
poolRewardInfo, found := k.GetPoolRewardInfo(ctx, poolId, rewardDenom)
if !found {
poolRewardInfo = types.PoolRewardInfo{
PoolId: poolId,
RewardDenom: rewardDenom,
PoolAccRewardPerShare: sdk.NewDec(0),
LastUpdatedBlock: 0,
}
}
userRewardInfo, found := k.GetUserRewardInfo(ctx, user, poolId, rewardDenom)
if !found {
userRewardInfo = types.UserRewardInfo{
User: user,
PoolId: poolId,
RewardDenom: rewardDenom,
RewardDebt: sdk.NewDec(0),
RewardPending: sdk.NewDec(0),
}
}
// need to consider balance change on deposit/withdraw
userBalance := k.GetPoolBalance(ctx, poolId, user)
if isDeposit {
userBalance = userBalance.Sub(amount)
} else {
userBalance = userBalance.Add(amount)
}
userRewardInfo.RewardPending = userRewardInfo.RewardPending.Add(
poolRewardInfo.PoolAccRewardPerShare.
Mul(math.LegacyNewDecFromInt(userBalance)).
Sub(userRewardInfo.RewardDebt).
Quo(math.LegacyNewDecFromInt(ammtypes.OneShare)),
)
k.SetUserRewardInfo(ctx, userRewardInfo)
}
func (k Keeper) UpdateUserRewardDebt(ctx sdk.Context, poolId uint64, rewardDenom string, user string) {
poolRewardInfo, found := k.GetPoolRewardInfo(ctx, poolId, rewardDenom)
if !found {
poolRewardInfo = types.PoolRewardInfo{
PoolId: poolId,
RewardDenom: rewardDenom,
PoolAccRewardPerShare: sdk.NewDec(0),
LastUpdatedBlock: 0,
}
}
userRewardInfo, found := k.GetUserRewardInfo(ctx, user, poolId, rewardDenom)
if !found {
userRewardInfo = types.UserRewardInfo{
User: user,
PoolId: poolId,
RewardDenom: rewardDenom,
RewardDebt: sdk.NewDec(0),
RewardPending: sdk.NewDec(0),
}
}
userRewardInfo.RewardDebt = poolRewardInfo.PoolAccRewardPerShare.Mul(
math.LegacyNewDecFromInt(k.GetPoolBalance(ctx, poolId, user)),
)
k.SetUserRewardInfo(ctx, userRewardInfo)
}
func TestHookMasterchef(t *testing.T) {
app, _, _ := simapp.InitElysTestAppWithGenAccount()
ctx := app.BaseApp.NewContext(true, tmproto.Header{})
mk, amm, oracle := app.MasterchefKeeper, app.AmmKeeper, app.OracleKeeper
// Setup coin prices
SetupStableCoinPrices(ctx, oracle)
authority := authtypes.NewModuleAddress(govtypes.ModuleName).String()
srv := tokenomicskeeper.NewMsgServerImpl(app.TokenomicsKeeper)
expected := &tokenomicstypes.MsgCreateTimeBasedInflation{
Authority: authority,
StartBlockHeight: uint64(1),
EndBlockHeight: uint64(6307200),
Inflation: &tokenomicstypes.InflationEntry{
LmRewards: 9999999,
IcsStakingRewards: 9999999,
CommunityFund: 9999999,
StrategicReserve: 9999999,
TeamTokensVested: 9999999,
},
}
wctx := sdk.WrapSDKContext(ctx)
_, err := srv.CreateTimeBasedInflation(wctx, expected)
require.NoError(t, err)
expected = &tokenomicstypes.MsgCreateTimeBasedInflation{
Authority: authority,
StartBlockHeight: uint64(6307201),
EndBlockHeight: uint64(12614401),
Inflation: &tokenomicstypes.InflationEntry{
LmRewards: 9999999,
IcsStakingRewards: 9999999,
CommunityFund: 9999999,
StrategicReserve: 9999999,
TeamTokensVested: 9999999,
},
}
_, err = srv.CreateTimeBasedInflation(wctx, expected)
require.NoError(t, err)
// Generate 1 random account with 1000stake balanced
addr := simapp.AddTestAddrs(app, ctx, 2, sdk.NewInt(10000000000))
// Create a pool
// Mint 100000USDC
usdcToken := sdk.NewCoins(sdk.NewCoin(ptypes.BaseCurrency, sdk.NewInt(10000000000)))
err = app.BankKeeper.MintCoins(ctx, ammtypes.ModuleName, usdcToken.MulInt(sdk.NewInt(2)))
require.NoError(t, err)
err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, ammtypes.ModuleName, addr[0], usdcToken)
require.NoError(t, err)
err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, ammtypes.ModuleName, addr[1], usdcToken)
require.NoError(t, err)
poolAssets := []ammtypes.PoolAsset{
{
Weight: sdk.NewInt(50),
Token: sdk.NewCoin(ptypes.Elys, sdk.NewInt(10000000)),
},
{
Weight: sdk.NewInt(50),
Token: sdk.NewCoin(ptypes.BaseCurrency, sdk.NewInt(10000000)),
},
}
argSwapFee := sdk.MustNewDecFromStr("0.0")
argExitFee := sdk.MustNewDecFromStr("0.0")
poolParams := &ammtypes.PoolParams{
SwapFee: argSwapFee,
ExitFee: argExitFee,
}
msg := ammtypes.NewMsgCreatePool(
addr[0].String(),
poolParams,
poolAssets,
)
// Create a Elys+USDC pool
poolId, err := amm.CreatePool(ctx, msg)
require.NoError(t, err)
require.Equal(t, poolId, uint64(1))
pools := amm.GetAllPool(ctx)
// check length of pools
require.Equal(t, len(pools), 1)
_, err = amm.ExitPool(ctx, addr[0], pools[0].PoolId, math.NewIntWithDecimal(1, 21), sdk.NewCoins(), "")
require.NoError(t, err)
// new user join pool with same shares
share := ammtypes.InitPoolSharesSupply.Mul(math.NewIntWithDecimal(1, 5))
t.Log(mk.GetPoolTotalCommit(ctx, pools[0].PoolId))
require.Equal(t, mk.GetPoolTotalCommit(ctx, pools[0].PoolId).String(), "10002000000000000000000000")
require.Equal(t, mk.GetPoolBalance(ctx, pools[0].PoolId, addr[0].String()).String(), "10000000000000000000000000")
_, _, err = amm.JoinPoolNoSwap(ctx, addr[1], pools[0].PoolId, share, sdk.NewCoins(sdk.NewCoin(ptypes.Elys, sdk.NewInt(10000000)), sdk.NewCoin(ptypes.BaseCurrency, sdk.NewInt(10000000))))
require.NoError(t, err)
require.Equal(t, mk.GetPoolTotalCommit(ctx, pools[0].PoolId).String(), "20002000000000000000000000")
require.Equal(t, mk.GetPoolBalance(ctx, pools[0].PoolId, addr[1].String()), share)
atomToken := sdk.NewCoins(sdk.NewCoin("uatom", math.NewIntWithDecimal(100000000, 6)))
err = app.BankKeeper.MintCoins(ctx, ammtypes.ModuleName, atomToken)
require.NoError(t, err)
err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, ammtypes.ModuleName, addr[0], atomToken)
require.NoError(t, err)
// external reward distribute
masterchefSrv := masterchefkeeper.NewMsgServerImpl(app.MasterchefKeeper)
_, err = masterchefSrv.AddExternalRewardDenom(sdk.WrapSDKContext(ctx), &types.MsgAddExternalRewardDenom{
Authority: app.GovKeeper.GetAuthority(),
RewardDenom: "uatom",
MinAmount: sdk.OneInt(),
Supported: true,
})
require.NoError(t, err)
_, err = masterchefSrv.AddExternalIncentive(sdk.WrapSDKContext(ctx), &types.MsgAddExternalIncentive{
Sender: addr[0].String(),
RewardDenom: "uatom",
PoolId: pools[0].PoolId,
AmountPerBlock: math.NewIntWithDecimal(100, 6),
FromBlock: 0,
ToBlock: 1000,
})
require.NoError(t, err)
// check rewards after 100 block
for i := 1; i <= 100; i++ {
mk.EndBlocker(ctx)
ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
}
require.Equal(t, ctx.BlockHeight(), int64(100))
poolRewardInfo, _ := app.MasterchefKeeper.GetPoolRewardInfo(ctx, pools[0].PoolId, "uatom")
require.Equal(t, poolRewardInfo.LastUpdatedBlock, uint64(99))
res, err := mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[0].String(),
})
require.NoError(t, err)
require.Equal(t, res.TotalRewards[0].Amount.String(), "4949505049")
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[1].String(),
})
require.NoError(t, err)
require.Equal(t, res.TotalRewards[0].Amount.String(), "4949505049")
// check rewards claimed
_, err = masterchefSrv.ClaimRewards(sdk.WrapSDKContext(ctx), &types.MsgClaimRewards{
Sender: addr[0].String(),
PoolIds: []uint64{pools[0].PoolId},
})
require.NoError(t, err)
_, err = masterchefSrv.ClaimRewards(sdk.WrapSDKContext(ctx), &types.MsgClaimRewards{
Sender: addr[1].String(),
PoolIds: []uint64{pools[0].PoolId},
})
require.NoError(t, err)
atomAmount := app.BankKeeper.GetBalance(ctx, addr[1], "uatom")
require.Equal(t, atomAmount.Amount.String(), "4949505049")
// no pending rewards
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[0].String(),
})
require.NoError(t, err)
require.Len(t, res.TotalRewards, 0)
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[1].String(),
})
require.NoError(t, err)
require.Len(t, res.TotalRewards, 0)
// first user exit pool
_, err = amm.ExitPool(ctx, addr[1], pools[0].PoolId, share.Quo(math.NewInt(2)), sdk.NewCoins(), "")
require.NoError(t, err)
// check rewards after 100 block
for i := 1; i <= 100; i++ {
mk.EndBlocker(ctx)
ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
}
require.Equal(t, ctx.BlockHeight(), int64(200))
poolRewardInfo, _ = app.MasterchefKeeper.GetPoolRewardInfo(ctx, pools[0].PoolId, "uatom")
require.Equal(t, poolRewardInfo.LastUpdatedBlock, uint64(199))
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[0].String(),
})
require.NoError(t, err)
require.Equal(t, res.TotalRewards[0].String(), "3999680025uatom")
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[1].String(),
})
require.NoError(t, err)
require.Equal(t, res.TotalRewards[0].String(), "1999840012uatom")
// check rewards claimed
_, err = masterchefSrv.ClaimRewards(sdk.WrapSDKContext(ctx), &types.MsgClaimRewards{
Sender: addr[0].String(),
PoolIds: []uint64{pools[0].PoolId},
})
require.NoError(t, err)
_, err = masterchefSrv.ClaimRewards(sdk.WrapSDKContext(ctx), &types.MsgClaimRewards{
Sender: addr[1].String(),
PoolIds: []uint64{pools[0].PoolId},
})
require.NoError(t, err)
atomAmount = app.BankKeeper.GetBalance(ctx, addr[1], "uatom")
require.Equal(t, atomAmount.String(), "6949345061uatom")
// no pending rewards
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[0].String(),
})
require.NoError(t, err)
require.Len(t, res.TotalRewards, 0)
res, err = mk.UserPendingReward(ctx, &types.QueryUserPendingRewardRequest{
User: addr[1].String(),
})
require.NoError(t, err)
require.Len(t, res.TotalRewards, 0)
pool, _ := mk.GetPool(ctx, pools[0].PoolId)
require.Equal(t, pool.ExternalIncentiveApr.String(), "4204.799481351999973502")
}
To mitigate these issues, it is recommended to modify the UpdateAccPerShare
function to initialize the LastUpdatedBlock
field with the current block height when creating a new PoolRewardInfo
struct. This ensures that the reward accumulation starts from the correct block, preventing potential reward inflation and distribution inconsistencies.
SOLVED: The Elys Network team solved this issue by using block.height.
// Critical
In the provided code snippet, there is a potential risk of the chain halting due to a panic that can occur when using the MustAccAddressFromBech32
function. Specifically, in the CollectGasFees
function, the following line of code is problematic:
protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)
The MustAccAddressFromBech32
function is used to convert a Bech32 encoded address string to an account address. However, if the provided Bech32 string is invalid or cannot be properly decoded, this function will panic.
If the params.ProtocolRevenueAddress
value is not a valid Bech32 encoded address, it will trigger a panic when passed to MustAccAddressFromBech32
. Since this code is executed in the CollectGasFees
function, which is likely called during the end block processing or related to fee distribution, a panic at this point can cause the entire chain to halt.
Panics in critical parts of the code, such as those related to block processing or consensus, can lead to a complete stoppage of the chain. All nodes running this code will encounter the panic and fail to proceed, effectively bringing the network to a halt.
To mitigate the risk of a chain halt due to a panic in MustAccAddressFromBech32
, it is recommended to use the AccAddressFromBech32
function instead and handle any potential errors gracefully. Here's an example of how to modify the code:
protocolRevenueAddress, err := sdk.AccAddressFromBech32(params.ProtocolRevenueAddress)
if err != nil {
// Handle the error appropriately, such as logging it and using a default address or skipping the fee distribution
ctx.Logger().Error("Invalid protocol revenue address", "error", err)
return
}
SOLVED: The Elys Network team solved the issue by using AccAddressFromBech32
as it was recommended.
// Critical
In the CollectGasFees
and CollectDEXRevenue
functions of the keeper module, there is a potential issue that could lead to a chain halt if the protocol revenue address is included in the block list.
The CollectGasFees
function transfers a portion of the collected gas fees to the protocol revenue address using the following code:
// Send coins to protocol revenue address
if protocolGasFeeCoins.IsAllPositive() {
protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)
err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, authtypes.FeeCollectorName, protocolRevenueAddress, protocolGasFeeCoins)
if err != nil {
panic(err)
}
}
Similarly, the CollectDEXRevenue
function transfers a portion of the DEX revenue to the protocol revenue address using the following code:
// Send coins to protocol revenue address
if protocolRevenueCoins.IsAllPositive() {
protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)
err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, protocolRevenueAddress, protocolRevenueCoins)
if err != nil {
panic(err)
}
}
The issue arises when the SendCoinsFromModuleToAccount
function is called. This function checks if the recipient address (in this case, the protocol revenue address) is included in the block list using the BlockedAddr
function. If the address is blocked, it returns an error wrapped with sdkerrors.ErrUnauthorized
.However, in both CollectGasFees
and CollectDEXRevenue
, if an error occurs during the coin transfer to the protocol revenue address, the code panics using panic(err)
. This means that if the protocol revenue address is blocked, it will trigger a panic and potentially halt the chain.
To address this issue and prevent a potential chain halt, the following recommendations can be considered:
1. Error Handling:
- Instead of using panic(err)
when an error occurs during the coin transfer to the protocol revenue address, the error should be properly handled and logged.
- The function can return the error to the caller, allowing it to be propagated and handled at a higher level.
SOLVED: The Elys Network team solved the issue by removing the problematic panics.
// High
In the AddExternalIncentive
function of the keeper
package, there is a potential integer overflow issue when calculating the total amount of incentives to be added. The specific line of concern is:
amount := msg.AmountPerBlock.Mul(sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)))
In this line, the difference between msg.ToBlock
and msg.FromBlock
is being converted to an int64
value using int64(msg.ToBlock - msg.FromBlock)
. If the result of msg.ToBlock - msg.FromBlock
exceeds the maximum value representable by int64
, which is 9,223,372,036,854,775,807, it will lead to an integer overflow.
func (k msgServer) AddExternalIncentive(goCtx context.Context, msg *types.MsgAddExternalIncentive) (*types.MsgAddExternalIncentiveResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
sender := sdk.MustAccAddressFromBech32(msg.Sender)
if msg.FromBlock < uint64(ctx.BlockHeight()) {
return nil, status.Error(codes.InvalidArgument, "invalid from block")
}
if msg.FromBlock >= msg.ToBlock {
return nil, status.Error(codes.InvalidArgument, "invalid block range")
}
if msg.AmountPerBlock.IsZero() {
return nil, status.Error(codes.InvalidArgument, "invalid amount per block")
}
found := false
params := k.GetParams(ctx)
for _, rewardDenom := range params.SupportedRewardDenoms {
if msg.RewardDenom == rewardDenom.Denom {
found = true
if msg.AmountPerBlock.Mul(
sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)),
).LT(rewardDenom.MinAmount) {
return nil, status.Error(codes.InvalidArgument, "too small amount")
}
break
}
}
if !found {
return nil, status.Error(codes.InvalidArgument, "invalid reward denom")
}
amount := msg.AmountPerBlock.Mul(sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)))
err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, sdk.Coins{sdk.NewCoin(msg.RewardDenom, amount)})
if err != nil {
return nil, err
}
k.Keeper.AddExternalIncentive(ctx, types.ExternalIncentive{
Id: 0,
RewardDenom: msg.RewardDenom,
PoolId: msg.PoolId,
FromBlock: msg.FromBlock,
ToBlock: msg.ToBlock,
AmountPerBlock: msg.AmountPerBlock,
Apr: math.LegacyZeroDec(),
})
return &types.MsgAddExternalIncentiveResponse{}, nil
}
To address the potential integer overflow issue, it is recommended to use the sdk.Int
type provided by the Cosmos SDK for arithmetic operations involving large values. The sdk.Int
type is designed to handle arbitrary-precision integers safely and prevents overflow issues.
SOLVED: The Elys Network team solved this issue by using int64 directly.
// High
The code snippet delegations := k.Keeper.Keeper.GetDelegatorDelegations(ctx, delAddr, 1024)
has a couple of issues related to delegation retrieval and error handling:
1. Hardcoded Limit:
- The code uses a hardcoded limit of 1024 when retrieving delegations from the keeper.
- While 1024 is a relatively large number, it assumes that the delegator will never have more than 1024 delegations.
- If the delegator exceeds this limit, the code will fail to retrieve all the delegations, potentially leading to incomplete reward withdrawal.2. Lack of
Error Handling:
- The code does not handle any errors that may occur during the retrieval of delegations using k.Keeper.Keeper.GetDelegatorDelegations
.
- If an error is returned by the keeper, it is not being captured or handled properly.
- Ignoring errors can lead to unexpected behavior and make it difficult to diagnose and fix issues.
To address these issues, consider the following recommendations:
1. Use a Dynamic or Configurable Limit:
- Instead of using a hardcoded limit, consider using a dynamic or configurable limit for retrieving delegations. (math.MaxUint16)
- The limit can be determined based on factors such as the maximum number of delegations allowed by the system or a configuration parameter.
- This allows for flexibility and adaptability to different scenarios and future changes.
2. Handle Errors Properly:
- Check and handle any errors returned by k.Keeper.Keeper.GetDelegatorDelegations
.
- If an error occurs during delegation retrieval, log the error and return an appropriate error response to the caller.
- Proper error handling helps in diagnosing issues and ensures that the system behaves correctly in case of failures.
SOLVED: The Elys Network team solved this issue by iterating delegations.
// Medium
The code snippet provided contains several instances of misleading error messages and comments that do not accurately reflect the functionality of the code. This can lead to confusion and difficulty in understanding the purpose and behavior of the functions.
In the msgClaimRewards
function:
1. The error message "failed to serialize stake" is incorrect. The function is actually serializing the response, not a stake.In the performMsgClaimRewards
function:
1. The error message "invalid address" is not descriptive enough. It should specify that the error occurs when parsing the sender address.
2. The error message "failed validating msgMsgDelegate" is incorrect. The function is validating msgMsgClaimRewards
, not msgMsgDelegate
.
3. The error message "elys redelegation msg" is incorrect. The function is performing a claim rewards operation, not a redelegation.
4. The response message "Redelegation succeed!" is incorrect. It should indicate that the claim rewards operation succeeded. These misleading error messages and comments can make it difficult for developers to understand the purpose and behavior of the code, leading to confusion and potential misinterpretation.
func performMsgClaimRewards(f *masterchefkeeper.Keeper, ctx sdk.Context, contractAddr sdk.AccAddress, msgClaimRewards *types.MsgClaimRewards) (*wasmbindingstypes.RequestResponse, error) {
if msgClaimRewards == nil {
return nil, wasmvmtypes.InvalidRequest{Err: "Invalid claim rewards parameter"}
}
msgServer := masterchefkeeper.NewMsgServerImpl(*f)
_, err := sdk.AccAddressFromBech32(msgClaimRewards.Sender)
if err != nil {
return nil, errorsmod.Wrap(err, "invalid address")
}
msgMsgClaimRewards := &types.MsgClaimRewards{
Sender: msgClaimRewards.Sender,
PoolIds: msgClaimRewards.PoolIds,
}
if err := msgMsgClaimRewards.ValidateBasic(); err != nil {
return nil, errorsmod.Wrap(err, "failed validating msgMsgDelegate")
}
_, err = msgServer.ClaimRewards(sdk.WrapSDKContext(ctx), msgMsgClaimRewards) // Discard the response because it's empty
if err != nil {
return nil, errorsmod.Wrap(err, "elys redelegation msg")
}
resp := &wasmbindingstypes.RequestResponse{
Code: paramtypes.RES_OK,
Result: "Redelegation succeed!",
}
return resp, nil
}
To improve the clarity and maintainability of the code, it is recommended to update the error messages and comments to accurately reflect the functionality of the code. Here are the suggested changes:1. In the msgClaimRewards
function:
- Update the error message to: "failed to serialize response"2. In the performMsgClaimRewards
function:
- Update the error message to: "invalid sender address"
- Update the error message to: "failed validating msgMsgClaimRewards"
- Update the error message to: "failed to process claim rewards"
- Update the response message to: "Claim rewards succeeded!"
SOLVED: The Elys Network team solved this issue by implementing the suggested recommendation.
// Low
The provided code snippet contains the implementation of the message handlers for various messages in the Cosmos SDK module. However, it does not emit any events during the execution of these message handlers. Event emission is a crucial aspect of Cosmos SDK modules as it allows other modules and external clients to react to state changes and perform specific operations based on the emitted events.
Other modules or external clients that rely on events emitted by this module will not be able to respond to state changes appropriately.
It becomes challenging to track and audit the execution of message handlers and state changes within the module.
The lack of event emission can lead to reduced transparency and auditability of the module's operations.
Consider implementing event emission in each message handler to ensure that other modules and external clients can react to state changes appropriately.
SOLVED: The Elys Network team solved this issue by implementing the suggested recommendation.
// Low
The spec file intends to outline the common structure for specifications within this directory. The masterchef/estaking module is 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.
SOLVED: The Elys Network team solved this issue by adding specs on the module.
// Low
The Elys App Chain, which utilizes a vulnerable version of CometBFT as its consensus engine, has been identified to have a default configuration issue. The default values for the EvidenceParams.MaxAgeNumBlocks
and EvidenceParams.MaxAgeDuration
consensus parameters may not provide sufficient coverage for the entire unbonding period of the chain. This could potentially lead to the premature expiration of evidence and allow for unpunished Byzantine behavior if evidence is discovered outside the defined window.
Affected Component:
CometBFT
Affected Versions:
The specific version of CometBFT used by the Elys App Chain, as mentioned in the go.mod
file:
```
github.com/cometbft/cometbft v0.37.4
```
It is recommended that chain ecosystems and their maintainers set the consensus parameters EvidenceParams.MaxAgeNumBlocks
and EvidenceParams.MaxAgeDuration
.
RISK ACCEPTED: The Elys Network team accepted the risk of the issue.
// Low
The Elys App Chain, which utilizes CometBFT as its consensus engine, has been identified to have a default configuration for BlockParams.MaxBytes
that may be too large for its specific use case. This default value can potentially increase block times and affect consensus participation when fully utilized by chain participants. It is recommended that the Noble App Chain team consider their specific needs and evaluate the impact of having proposed blocks with the maximum allowed block size, especially on bandwidth usage and block latency.
To mitigate the potential impact of this default configuration, it is recommended that the Elys App Chain team take the following step:
1. Evaluate the specific needs and requirements of the Noble App Chain use case and determine an appropriate value for BlockParams.MaxBytes
.
SOLVED: The Elys Network team solved the issue by adding the following lines:
consensusParams.Block.MaxBytes = NewMaxBytes app.ConsensusParamsKeeper.ParamsStore.Set(ctx, consensusParams)
// Informational
The Command Line Interface (CLI) for the module is currently lacking long descriptions, which are available in the Cosmos SDK. Long descriptions provide users with detailed information about the purpose and usage of CLI commands. The absence of these descriptions reduces the overall usability and user experience of the CLI, as users may face difficulty in understanding the functionality and proper usage of various commands.
To address the issue of missing long descriptions in the CLI, it is recommended to:
Update the CLI to include long descriptions for all commands, following the guidelines and format provided by the Cosmos SDK.
Ensure that these descriptions are comprehensive and accurately convey the purpose and usage of each command.
ACKNOWLEDGED: The Elys Network team acknowledged the issue.
// Informational
The ConsensusVersion()
function in the AppModule
of the estaking
module is currently set to return a value of 1.
// ConsensusVersion is a sequence number for state-breaking change of the module. It should be incremented on each consensus-breaking change introduced by the module. To avoid wrong/empty versions, the initial version should be set to 1
func (AppModule) ConsensusVersion() uint64 { return 1 }
Review the estaking
module and identify any consensus-breaking changes that have been introduced since the initial version. Consider incrementing it by one.
ACKNOWLEDGED: The Elys Network team acknowledged the issue.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed