refactor: Cover high level audit items - #9
sontrinh16 wants to merge 7 commits into
Conversation
hieuvubk
left a comment
hieuvubk
left a comment
There was a problem hiding this comment.
1st review. Overall look good to me.
1 thing I dont understand was about burning in UnescrowCoinTo
Also let make a separate pr to realio-network repo to port multistaking commit hash into it. Make sure integration test in realio-network passed. Also let add more test case into it to prove changes works.
| ConvertCoin(ctx context.Context, msg *erc20types.MsgConvertCoin) (*erc20types.MsgConvertCoinResponse, error) | ||
| ConvertERC20(ctx context.Context, msg *erc20types.MsgConvertERC20) (*erc20types.MsgConvertERC20Response, error) | ||
| RegisterERC20(ctx context.Context, msg *erc20types.MsgRegisterERC20) (*erc20types.MsgRegisterERC20Response, error) | ||
| GetTokenPair(ctx sdk.Context, id []byte) (erc20types.TokenPair, bool) |
There was a problem hiding this comment.
why we need to expose this func at multistaking keeper?
There was a problem hiding this comment.
for the check after convert erc20 token where a deleted contract cause the erc20 keeper to remove the token pairing
| if len(tokenID) == 0 && erc20types.ValidateErc20Denom(coin.Denom) == nil { | ||
| // A previous conversion may already have deleted the dead contract's | ||
| // token pair. Its remaining wrapped coins must not become native payouts. | ||
| return k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins) |
There was a problem hiding this comment.
what is burn coins step here and after erc20 conversion about?
There was a problem hiding this comment.
just pre check burn for already removed contracts but the wrapped token still exists
| // selfdestructed contract. Only the latter deletes the pair and leaves | ||
| // the Cosmos coins unburned in the recipient's account. | ||
| if _, found := k.erc20keeper.GetTokenPair(sdkCtx, tokenID); !found { | ||
| if err := k.BurnCoin(sdkCtx, toAcc, coin); err != nil { |
There was a problem hiding this comment.
this to cover pair that got delete during the conversion
When an ERC20 contract is selfdestructed after delegation, |
make sense, let add an integration test for it to explain to realio team later |
2 participants
Summary
Address audit findings affecting ERC20 withdrawals, staking amount validation, governance proposals, and genesis initialization.
Changes
GetTokenPairto the ERC20 keeper interface and update the test mock.RemoveMultiStakingCoinProposal.SetValidatorMultiStakingCoinand update callers to handle them.Testing
CGO_ENABLED=0 go test -tags=test ./x/multi-staking/types ./x/multi-staking/keeperCompatibility
SetValidatorMultiStakingCoinnow returns an error. Downstream callers should handle it.InitGenesisstill panics on invalid input if genesis validation is bypassed.