Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix misbehaviour handling for solo machine #7515

Merged
merged 10 commits into from
Oct 13, 2020
1 change: 1 addition & 0 deletions proto/ibc/lightclients/solomachine/v1/solomachine.proto
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ message SignatureAndData {
bytes signature = 1;
DataType data_type = 2 [(gogoproto.moretags) = "yaml:\"data_type\""];
bytes data = 3;
uint64 timestamp = 4;
}

// TimestampedSignatureData contains the signature data and the timestamp of the
Expand Down
3 changes: 3 additions & 0 deletions x/ibc/light-clients/06-solomachine/types/misbehaviour.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ func (sd SignatureAndData) ValidateBasic() error {
if sd.DataType == UNSPECIFIED {
return sdkerrors.Wrap(ErrInvalidSignatureAndData, "data type cannot be UNSPECIFIED")
}
if sd.Timestamp == 0 {
return sdkerrors.Wrap(ErrInvalidSignatureAndData, "timestamp cannot be 0")
}

return nil
}
11 changes: 9 additions & 2 deletions x/ibc/light-clients/06-solomachine/types/misbehaviour_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,23 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(

// verifySignatureAndData verifies that the currently registered public key has signed
// over the provided data and that the data is valid. The data is valid if it can be
// unmarshaled into the specified data type.
// unmarshaled into the specified data type or the timestamp of the signature is less
// than the consensus state timestamp.
func verifySignatureAndData(cdc codec.BinaryMarshaler, clientState ClientState, misbehaviour *Misbehaviour, sigAndData *SignatureAndData) error {
// timestamp less than consensus state would always fail and not succeed in fooling the
// light client
if sigAndData.Timestamp < clientState.ConsensusState.Timestamp {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @cwgoes this differs from the spec, but I think it is correct. Should we keep this check? It just stops clients from being frozen if the timestamp signed was not recent enough to fool the light client into passing verification or update checks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense to me. Thanks for the ping. Spec changes made in cosmos/ibc@128082e.

return sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "timestamp is less than consensus state timestmap (%d < %d)", sigAndData.Timestamp, clientState.ConsensusState.Timestamp)
}

// ensure data can be unmarshaled to the specified data type
if _, err := UnmarshalDataByType(cdc, sigAndData.DataType, sigAndData.Data); err != nil {
return err
}

data, err := MisbehaviourSignBytes(
cdc,
misbehaviour.Sequence, clientState.ConsensusState.Timestamp,
misbehaviour.Sequence, sigAndData.Timestamp,
clientState.ConsensusState.Diversifier,
sigAndData.DataType,
sigAndData.Data,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,34 @@ func (suite *SoloMachineTestSuite) TestCheckMisbehaviourAndUpdateState() {
misbehaviour = m
}, false,
},
{
"invalid SignatureOne timestamp",
func() {
clientState = solomachine.ClientState()
m := solomachine.CreateMisbehaviour()

m.SignatureOne.Timestamp = 1000000000000
misbehaviour = m
}, false,
},
{
"invalid SignatureTwo timestamp",
func() {
clientState = solomachine.ClientState()
m := solomachine.CreateMisbehaviour()

m.SignatureTwo.Timestamp = 1000000000000
misbehaviour = m
}, false,
},
{
"timestamp is less than consensus state timestamp",
func() {
clientState = solomachine.ClientState()
solomachine.Time = solomachine.Time - 5
misbehaviour = solomachine.CreateMisbehaviour()
}, false,
},
{
"invalid first signature data",
func() {
Expand Down
12 changes: 12 additions & 0 deletions x/ibc/light-clients/06-solomachine/types/misbehaviour_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,18 @@ func (suite *SoloMachineTestSuite) TestMisbehaviourValidateBasic() {
misbehaviour.SignatureTwo.DataType = types.UNSPECIFIED
}, false,
},
{
"timestamp for SignatureOne is zero",
func(misbehaviour *types.Misbehaviour) {
misbehaviour.SignatureOne.Timestamp = 0
}, false,
},
{
"timestamp for SignatureTwo is zero",
func(misbehaviour *types.Misbehaviour) {
misbehaviour.SignatureTwo.Timestamp = 0
}, false,
},
}

for _, tc := range testCases {
Expand Down
Loading