code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

updateValset operation doesn't set a timeout, leading to the signed newCheckpoint (new Valset) being used indefinitely.

Proof of Concept

It doesn't check the operation timeout in the updateValset function, so the new Valset can be delayed and change the current Valset abruptly at any time.

Additionally, submitBatch and submitLogicCall function check the checkpoint:

require(
    makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint,
    "Supplied current validators and powers do not match checkpoint."
);

An attacker can hold the newCheckpoint and call updateValset abruptly to change validators list to deny all valid operations, which has been signed by previous _currentValset.validators.

Tools Used

vim

Recommended Mitigation Steps

Use timeout in updateValset:

    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 {
        // CHECKS

+       require(block.number < _newValset.updateTimeout, "Timed out");

        ...

V-Staykov commented 2 years ago

The update valset function is being called by all the orchestrators and basically the first of them to pass is being stored. So if a validator decides to hold a updateValset transaction he doesn't do any harm oher than making other validators incur the fees of the transaction. Also each valsetupdate has an once which would prevent overiding an update that was sent after it, since its nonce would be higher.

albertchon commented 2 years ago

Agreed with @V-Staykov

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