Cudos contest - cccz's results

Lines of code

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

Vulnerability details

Impact

Same as https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/63

The code does not enforce that the sum of validator powers is no less than the threshold. It is possible that even when all validators sign the message their total power is not enough to confirm it.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Same as the fix below: https://github.com/umee-network/peggo/pull/77/commits/e08dbee3abd89c82b3e121fe38e8ab71a96a1959

  function updateValset(
    // The new version of the validator set
    ValsetArgs memory _newValset,
    // The current validators that approve the change
    ValsetArgs memory _currentValset,
    // These are arrays of the parts of the current validator's signatures
    uint8[] memory _v,
    bytes32[] memory _r,
    bytes32[] memory _s
  ) external whenNotPaused {
    // CHECKS
    // Check that the valset nonce is greater than the old one
    require(
      _newValset.valsetNonce > _currentValset.valsetNonce,
      "New valset nonce must be greater than the current nonce"
    );
    // Check that new validators and powers set is well-formed
    require(
      _newValset.validators.length == _newValset.powers.length,
      "Malformed new validator set"
    );
    // Check that current validators, powers, and signatures (v,r,s) set is well-formed
    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"
    );
    // Check that the supplied current validator set matches the saved checkpoint
    require(
      makeCheckpoint(_currentValset, state_peggyId) ==
        state_lastValsetCheckpoint,
      "Supplied current validators and powers do not match checkpoint."
    );

+    uint256 state_powerThreshold_ = state_powerThreshold;
+    uint256 cumulativePower;
+		for (uint256 i = 0; i < _newValset.powers.length; i++) {
+			cumulativePower = cumulativePower + _newValset.powers[i];
+			if (cumulativePower > state_powerThreshold_) {
+				break;
+			}
+		}
+		require(
+			cumulativePower > state_powerThreshold_,
+			"New validator set signatures do not have enough power." 
+		);

Lines of code

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

Vulnerability details

Impact

The sendToCosmos function of Gravity transfers _amount of _tokenContract from the sender using the function safeTransferFrom. If the transferred token is a transfer-on-fee/deflationary token, the actually received amount could be less than _amount. However, since _amount is passed as a parameter of the SendToCosmosEvent event, the Cosmos side will think more tokens are locked on the Ethereum side.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider getting the received amount by calculating the difference of token balance (using balanceOf) before and after the transferFrom.

Lines of code

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

Vulnerability details

Impact

The verifySig function of Gravity calls the Solidity ecrecover function directly to verify the given signatures. The return value of ecrecover may be 0, which means the signature is invalid, but the check can be bypassed when signer is 0.

	function verifySig(
		address _signer,
		bytes32 _theHash,
		uint8 _v,
		bytes32 _r,
		bytes32 _s
	) private pure returns (bool) {
		bytes32 messageDigest = keccak256(
			abi.encodePacked("\x19Ethereum Signed Message:\n32", _theHash)
		);
		return _signer == ecrecover(messageDigest, _v, _r, _s);
	}

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Use the recover function from OpenZeppelin's ECDSA library for signature verification.