Cudos contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L276-L358

Vulnerability details

Impact

The updateValset function don't check that the sum of the powers of the new validators in the new valset is greater than the threshold, which can lead to unwanted behavior.

There are 2 main problems that can occur in that situation:

1. The sum of the new validators' powers will be lower than the state_powerThreshold
2. The sum of the new validators' power will overflow and become lower than the state_powerThreshold

The second case is less dangerous, because it won't stuck the system in every case (only in specific cases where every sum of validators' power is less than the threshold). The first case is very dangerous though. It can lead to the system becoming stuck and to all of the tokens on the cudos chain to become locked for users, because the validators won't have enough power to approve any operation - whether it is transferring tokens or updating the valset.

Proof of Concept

For the first case, consider the current validators set containing 100 validators with each ones power being equal to 10, and the threshold is 900 (91+ validators are needed for approvement). Now the updateValset function is being called with 100 validators with each ones power being equal to 1. This will lead to a state where no matter how much validators have signed a message, the sum of the powers won't pass the threshold and the action won't be able to be executed. This will cause all the tokens in the cudos blockchain become locked, and will DoS all the actions of the gravity contract - including updating the valset.

For the second case, consider the new validators set will have 128 validators, each validator's power is equal to 2**249 and _powerThreshold = 2**256 - 1. In this case the system will be stuck too, because every sum of validators' power won't pass the threshold.

Tools Used

Remix and VS Code

Recommended Mitigation Steps

Add a check in the updateValset to assure that the sum of the new powers is greater than the threshold.

Lines of code

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

Vulnerability details

Impact

Lack of check that _tokenContract is a supported token in sendToCosmos can use users to lose money.

Proof of Concept

If a user calls sendToCosmos with a non supported token , then the transferFrom transaction in sendToCosoms would happened but the user won't receive the tokens back on the cosmos chain because this token is not supported there. So it is better to verify that the bridge actually support this token before taking it from the user.

Tools Used

Recommended Mitigation Steps

Verify that the bridge actually support this token before taking it from the user.

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L647

Vulnerability details

Impact

The constructor (and the updateValset function) of the Gravity contract receives the validators list as an array of addresses. By mistake, a validator can appear in this array twice, giving him multiple voting power.

Another thing that can happen is that the other validators will hold a list of validators that won't have that duplicate validator, and that can make the system fail (wrong signatures, and even wrong currentValset parameter).

Proof of Concept

Consider calling the constructor with _validators = [0x4bCc7c54E4e8E9a563B92272A51bBFb5d233de32, 0x4bCc7c54E4e8E9a563B92272A51bBFb5d233de32, 0x1EEf62fb64Ed62690E8e4833C5C2fe0d97e2B608] (Both of these addresses are mine, feel free to send tokens to it), _powers = [10, 10, 10] and _powerThreshold = 15. Normally, we would have wanted that the two validators will need to sign the transactions in order to approve the action, but in this case the validator sitting in 0x4bCc7c54E4e8E9a563B92272A51bBFb5d233de32 address can sign the message and approve the action without waiting to the second validator to sign it.

Tools Used

Remix and VS Code

Recommended Mitigation Steps

Check that the given validators array doesn't contains duplicates, or alternatively use the EnumerableSet.AddressSet data structures.

Gas Optimizations

• for loop in Gravity (write about the one in checkValidatorSignatures)

• Loop optimizations can be applied on many loops in the code. Let's take the for loop in the checkValidatorSignatures function for example.

  for (uint256 i = 0; i < _currentValidators.length; i++) {
  	// If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation
  	// (In a valid signature, it is either 27 or 28)
  	if (_v[i] != 0) {
  		// Check that the current validator has signed off on the hash
  		require(
  			verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]),
  			"Validator signature does not match."
  		);
  
  		// Sum up cumulative power
  		cumulativePower = cumulativePower + _currentPowers[i];
  
  		// Break early to avoid wasting gas
  		if (cumulativePower > _powerThreshold) {
  			break;
  		}
  	}
  }

  we can apply multiple optimizations to this loop:

  1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So for example in the loop above, the code can be optimized using uint i; instead of uint i = 0;.
  2. Use ++i instead of i++ to save some gas spent in every iteration.
  3. Save the array length in a local variable before the loop instead of accessing it in every iteration.
  4. Save _v[i] in a local variable instead of accessing the i element of the array twice in every iteration.

  so after applying all these changes, the code will look something like this:

  uint length = _currentValidators.length;
  for (uint256 i; i < length; ++i) {
  	// If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation
  	// (In a valid signature, it is either 27 or 28)
      uint8 v = _v[i];
  	if (v != 0) {
  		// Check that the current validator has signed off on the hash
  		require(
  			verifySig(_currentValidators[i], _theHash, v, _r[i], _s[i]),
  			"Validator signature does not match."
  		);
  
  		// Sum up cumulative power
  		cumulativePower = cumulativePower + _currentPowers[i];
  
  		// Break early to avoid wasting gas
  		if (cumulativePower > _powerThreshold) {
  			break;
  		}
  	}
  }

  Optimizations of this kind can be done to other loops in the code, and will reduce their cost.

• Redundant initializing of cumulativePower in the checkValidatorSignatures function (uint256 cumulativePower = 0; - line 231)

• Use the calldata modifier instead of memory when passing arrays as function parameters to reduce the gas cost of the function call

• Combine the checkValidatorSignatures and isOrchestrator functions to avoid 2 loops (the new function can be a different function from checkValidatorSignatures to avoid checking it whenever the isOrchestrator check is not needed)

• Save the length in the updateValset function instead of accessing it 4 times (every access is 3 memory touches). This can be done also in the submitBatch and submitLogicCall functions.

  old code:

  require(
      _currentValset.validators.length == _currentValset.powers.length &&
      _currentValset.validators.length == _v.length &&
      _currentValset.validators.length == _r.length &&
      _currentValset.validators.length == _s.length,
      "Malformed current validator set"
  );

  new code:

  uint length = _currentValset.validators.length;
  require(
      length == _currentValset.powers.length &&
      length == _v.length &&
      length == _r.length &&
      length == _s.length,
      "Malformed current validator set"
  );

• Save 2 SLOADs in updateValset function by saving the state_lastEventNonce variable

  old code:

  state_lastEventNonce = state_lastEventNonce.add(1);
  emit ValsetUpdatedEvent(
  	_newValset.valsetNonce,
  	state_lastEventNonce,
  	_newValset.rewardAmount,
  	_newValset.rewardToken,
  	_newValset.validators,
  	_newValset.powers
  );

  new code:

  uint256 lastEventNonce = state_lastEventNonce;
  state_lastEventNonce = lastEventNonce.add(1);
  emit ValsetUpdatedEvent(
  	_newValset.valsetNonce,
  	lastEventNonce,
  	_newValset.rewardAmount,
  	_newValset.rewardToken,
  	_newValset.validators,
  	_newValset.powers
  );

• Use return instead if break in the checkValidatorSignatures function to prevent checking the same condition twice (once in the if statement inside the loop and the other one in the require)

  function checkValidatorSignatures(
  	// The current validator set and their powers
  	address[] memory _currentValidators,
  	uint256[] memory _currentPowers,
  	// The current validator's signatures
  	uint8[] memory _v,
  	bytes32[] memory _r,
  	bytes32[] memory _s,
  	// This is what we are checking they have signed
  	bytes32 _theHash,
  	uint256 _powerThreshold
  ) private pure {
  	uint256 cumulativePower = 0;
  
  	for (uint256 i = 0; i < _currentValidators.length; i++) {
  		// If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation
  		// (In a valid signature, it is either 27 or 28)
  		if (_v[i] != 0) {
  			// Check that the current validator has signed off on the hash
  			require(
  				verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]),
  				"Validator signature does not match."
  			);
  
  			// Sum up cumulative power
  			cumulativePower = cumulativePower + _currentPowers[i];
  
  			// Break early to avoid wasting gas
  			if (cumulativePower > _powerThreshold) {
  				break; // use return instead
  			}
  		}
  	}
  
  	// Check that there was enough power
  	require(
  		cumulativePower > _powerThreshold,
  		"Submitted validator set signatures do not have enough power."
  	);
  	// Success
  }

• Redundant initialization of state_lastValsetNonce to zero - uint256 public state_lastValsetNonce = 0;

• Use type(uint256).max instead of the storage variable MAX_UINT in CosmosToken to save gas (this optimization saves an SLOAD and reduces the gas of that operation)

• Use a custom function that uses ++ (prefix) instead of state_lastEventNonce = state_lastEventNonce.add(1)

• Save an SLOAD by saving the state_gravityId variable in a local variable in the updateValset function

  old code:

  // Check that the supplied current validator set matches the saved checkpoint
  require(
      makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint,
      "Supplied current validators and powers do not match checkpoint."
  );
  
  require(
      isOrchestrator(_currentValset, msg.sender),
      "The sender of the transaction is not validated orchestrator"
  );
  
  // Check that enough current validators have signed off on the new validator set
  bytes32 newCheckpoint = makeCheckpoint(_newValset, state_gravityId);

  new code:

  bytes32 gravityId = state_gravityId;
  
  // Check that the supplied current validator set matches the saved checkpoint
  require(
      makeCheckpoint(_currentValset, gravityId) == state_lastValsetCheckpoint,
      "Supplied current validators and powers do not match checkpoint."
  );
  
  require(
      isOrchestrator(_currentValset, msg.sender),
      "The sender of the transaction is not validated orchestrator"
  );
  
  // Check that enough current validators have signed off on the new validator set
  bytes32 newCheckpoint = makeCheckpoint(_newValset, gravityId);