vegaprotocol / vega

A Go implementation of the Vega Protocol, a protocol for creating and trading derivatives on a fully decentralised network.
https://vega.xyz
GNU Affero General Public License v3.0
37 stars 21 forks source link

Review validation on all command inputs #8004

Closed gordsport closed 1 year ago

gordsport commented 1 year ago

Issue

There was a mainnet incident that caused an outage whereby there was insufficient validation on the Tendermint public keys specified in the announce node command, when this validator was promoted to become a consensus validator it caused a panic.

We need to review ALL the command inputs and ensure that we have sufficient validation to ensure that malformed data is not input.

Tasks

wwestgarth commented 1 year ago

Ethereum address validation could be tightened up. We use this function to convert ethereum address to a canonical form:

// EthereumChecksumAddress is a simple utility function
// to ensure all ethereum addresses used in vega are checksumed
// this expects a hex encoded string.
func EthereumChecksumAddress(s string) string {
    // as per docs the Hex method return EIP-55 compliant hex strings
    return common.HexToAddress(s).Hex()
}

but we do not check it is a hex string first. So if I pass in hello I get the null address:

EthereumChecksumAddress("hello") -> 0x0000000000000000000000000000000000000000

I would expect some checkTx error on commands that pass in a eth address (eg. announce node, issue signatures, withdrawals) rather than using 0x0000000000000000000000000000000000000000.

wwestgarth commented 1 year ago

Potential for panics if signatures aren't given when announcing a node. In VerifyAnnounceNode we .Value what could be nil. Need to add a nil check in the command validation.

Also need to check what happens if we set fromEpoch to an epoch way in the past, and (less critically but worth thinking about) setting to to an epoch way way into the future.

wwestgarth commented 1 year ago

Talking with @jgsbennett we've identified the following areas to target:

gordsport commented 1 year ago

Closing as this issue has uncovered a number of areas that have needed attention. These have been resolved.