No limit on nonces can cause Gravity bridge to be bricked

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L290 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L385 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L495

Vulnerability details

Impact

The purpose of the nonces in the Gravity bridge are to ensure that old confirmed transactions can't be resubmitted and mutate state again. To ensure this doesn't occur the contract checks to verify that the new nonce is greater than the old nonce. However, it doesn't set an upper bound for the nonce. Thus, it is theoretically possible for the nonces to be set to MAX_UINT, at which point all calls to the relevant contract methods will fail. This situation would require the validators to sign a transaction with the offending nonce, but if this did occur the bridge would be unable to facilitate further transfers.

Proof of Concept

The contract uses the following states that keep track of nonces that are passed in as arguments:

state_lastValsetNonce
state_lastBatchNonces
state_invalidationMapping

For the methods submitLogicCall, submitBatch and updateValset, they all have a similar check that validates whether the new nonce passed in as an argument is greater than the previously stored nonce, for example:

require(
  _newValset.valsetNonce > _currentValset.valsetNonce,
  "New valset nonce must be greater than the current nonce"
);

The state is then updated to the new nonce value:

state_lastValsetNonce = _newValset.valsetNonce;

If the new nonce were MAX_UINT then the call would pass, but the following call would always fail since no number can be greater than MAX_UINT.

Tools Used

VSCode

Recommended Mitigation Steps

Depending on how aggressive you want the check on the new nonces to be, you can either:

1. Add another require statement to each of the 3 methods mentioned above checking that the new nonce is 1 greater than the previous nonce. e.g.

require(_newValset.valsetNonce == state_lastValsetNonce.add(1));

2. Set a modifiable range in which the new nonce can reside. e.g.

uint public nonceRange;

function setNonceRange(uint newRange) external {
  require(cudosAccessControls.hasAdminRole(msg.sender));
  nonceRange = newRange;
}

and then update the relevant require statements in the previously mentioned methods. e.g.

require(_newValset.valsetNonce > _currentValset.valsetNonce && _newValset.valsetNonce <= _currentValset.valsetNonce.add(nonceRange))

[V-Staykov]

While this might seem as an issue if you look at the contract as a standalone, it is not when you look at it as part of the solution, working with the gravity module. The batch nonce is automatically incremented in the cosmos Gravity module, so unless we have malicious validators with higher than the threshold power, this would not happen.

The code for the batch construction can be found here: https://github.com/code-423n4/2022-05-cudos/blob/main/module/x/gravity/keeper/batch.go#L53