code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Contract Gravity does not protect against setting empty list of validators. There should be no case when the number of signers in the bridge is set to empty list.

Proof of Concept

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check for empty set of number of signers. updateValset:

require(
    _newValset.validators.length != 0 && _newValset.validators.length == _newValset.powers.length,
    "Malformed new validator set"
);

constructor:

require(_validators.length != 0 && _validators.length == _powers.length, "Malformed current validator set");

V-Staykov commented 2 years ago

Such check is not actually needed, because if an empty valdiator set can only be signed and sent by valdiators in the current validator set. That means that the new valdiator set can be empty only if it is really empty on the Gravity module side.

Now having that in mind, having an empty validator set on the module side is a legit case and would mean the network has stopped. That is why I think this is not a problem for the contract - empty valdiator set is a legit case for the contract.

albertchon commented 2 years ago

Agreed with @V-Staykov

JeeberC4 commented 2 years ago

Creating warden's QA Report as judge downgraded issue. Preserving original title: Gravity - lack of check for empty set of signers