Cudos contest - hubble's results

Lines of code

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

Vulnerability details

Impact

When the cumulative power of validators in _newValset is less than or equal to state_powerThreshold, the checkValidatorSignatures function would fail. Eventually, submitBatch, submitLogicCall & updateValset would fail for the new set of validators. This will cause the contract non-functional, as updateValset can not be updated anymore with any new set of validators to fix it.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Add the following validation code fix in updateValset function.

// Check cumulative power to ensure the contract has sufficient power to actually
// pass a vote
uint256 cumulativePower = 0;
for (uint256 i = 0; i < _newValset.powers.length; i++) {
  cumulativePower = cumulativePower + _newValset.powers[i];
  if (cumulativePower > state_powerThreshold) {
    break;
  }
}
require(
  cumulativePower > state_powerThreshold,
  "Submitted validator set signatures do not have enough power."
);

Summary of Findings for Low / Non-Critical issues

• L-01 : Direct usage of ecrecover allows signature malleability
• L-02 : SafeMath library is not always used in Gravity.sol
• L-03 : Input validation of constructor parameters in Gravity.sol
• L-04 : Use structure for signature
• L-05 : Use constant for MAX_UINT

Details L-01

Title : Direct usage of ecrecover allows signature malleability

Impact

The verifySig function of Gravity.sol calls the Solidity ecrecover function directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice (and so is checking _signer != address(0), where address(0) means an invalid signature).

Proof of Concept

Contract : Gravity.sol Function : verifySig

Recommended Mitigation Steps

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

Details L-02

Title : SafeMath library is not always used in Gravity.sol

Impact

SafeMath library functions are not always used in the Gravity contract's arithmetic operations, which could cause integer underflow/overflows. Using SafeMath is considered a best practice that could completely prevent underflow/overflows and increase code consistency.

Proof of Concept

Contract : Gravity.sol Function : checkValidatorSignatures(...) line 244 : cumulativePower = cumulativePower + _currentPowers[i];

Function : constructor(...) line 661 : cumulativePower = cumulativePower + _powers[i];

Recommended Mitigation Steps

Consider using the SafeMath library functions in the above mentioned lines of code.

Details L-03

Title : Input validation of constructor parameters in Gravity.sol

In the constructor additional check for _gravityId not to be null, and _powerThreshold to be greater than 0, can be added.

Proof of Concept

Contract : Gravity.sol Function : constructor (...)

Recommended Mitigation Steps

Add appropriate require statement checking the input parameters mentioned above for the constructor

Details L-04

Title : Use structure for signature

Add a struct "Signature" which contains the v, r, and s components of a validator signature, This is a more compact encoding and removes the possibility of mismatched signature array lenghts.

Furthermore this reduces our total stack usage and allows us to declare validator sets and signatures to be of type 'calldata' universally.

// This represents a validator signature struct Signature { uint8 v; bytes32 r; bytes32 s; }

Proof of Concept

Contract : Gravity.sol Functions : testCheckValidatorSignatures, verifySig, checkValidatorSignatures, updateValset, submitBatch, submitLogicCall

Recommended Mitigation Steps

Refer to the code fix in https://github.com/Gravity-Bridge/Gravity-Bridge/commit/2bd0837b3ef52fe2f2abc705fc72f371e047c857

Details L-05

Title : Use constant for MAX_UINT

Use constants to improve security by preventing assignments and increase performance by explicitly flagging this constant.

Proof of Concept

Contract : CosmosERC20 in CosmosToken.sol Line 5 : uint256 MAX_UINT = 2**256 - 1;

Recommended Mitigation Steps

Change the above line to

uint256 constant MAX_UINT = 2**256 - 1;