feat(rollapp): move DRS version to be part of the block descriptor #1311
Conversation
There was a problem hiding this comment.
unbound iteration and missing validateBasic validation to the DRS.
related to https://github.com/dymensionxyz/research/issues/426
update: I see there is a todo about the DRS version validation. not sure why as we can still support for backeward compatability as it will be empty in the backward case.
// TODO: add a validation for DrsVersion once empty DRS version is marked vulnerable
// https://github.com/dymensionxyz/dymension/issues/1233
|
waiting to merge until dymensionxyz/dymint#1130 is merged |
omritoptix
changed the title
| // We only check first and last BD to avoid DoS attack related to iterating big number of BDs (taking into account a state update can be submitted with any numblock value) | ||
| // It is assumed there cannot be two upgrades in the same state update (since it requires gov proposal), if this happens it will be a fraud caught by Rollapp validators. | ||
| // Therefore checking first and last BD for deprecated DRS version should be enough. | ||
| var bdsToCheck []*types.BlockDescriptor | ||
| bdsToCheck = append(bdsToCheck, &msg.BDs.BD[0]) | ||
| if msg.NumBlocks > 1 { | ||
| bdsToCheck = append(bdsToCheck, &msg.BDs.BD[len(msg.BDs.BD)-1]) | ||
| } | ||
| for _, bd := range bdsToCheck { | ||
| // verify the DRS version is not vulnerable | ||
| if k.IsDRSVersionVulnerable(ctx, bd.DrsVersion) { | ||
| // the rollapp is not marked as vulnerable yet, mark it now | ||
| err := k.MarkRollappAsVulnerable(ctx, msg.RollappId) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("mark rollapp vulnerable: %w", err) | ||
| } | ||
| k.Logger(ctx).With("rollapp_id", msg.RollappId, "drs_version", bd.DrsVersion). | ||
| Info("non-frozen rollapp tried to submit MsgUpdateState with the vulnerable DRS version, mark the rollapp as vulnerable") | ||
| // we must return non-error if we want the changes to be saved | ||
| return &types.MsgUpdateStateResponse{}, nil |
There was a problem hiding this comment.
nit: can this be DRYer wrt to the mark_vulnerable.. ?
lots of dupe here
also ditto my comment about checking first and last
There was a problem hiding this comment.
not sure what you mean by DRYer here...
There was a problem hiding this comment.
reducing code duplications https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
| // the rollapp is not marked as vulnerable yet, mark it now | ||
| err := k.MarkRollappAsVulnerable(ctx, msg.RollappId) |
There was a problem hiding this comment.
ah I guess this is a store read
could do a happy path optimization to avoid rechecking ones which are already seenf
but IDK if we want to microoptimize
Description
Closes #1295
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
For Reviewer:
After reviewer approval: