code-423n4 / 2022-05-cudos-findings

1 stars 0 forks source link

Validator set can be set to empty #131

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

There lack check to make sure _newValset is not an empty array. Validator set can be set to empty and fund would be lost.

Proof of Concept

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

    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
    ) public nonReentrant {

Recommended Mitigation Steps

require(_newValset.length != 0);
V-Staykov commented 2 years ago

Duplicate of #172

albertchon commented 2 years ago

Agreed with the comment on #172 that

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.