search

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

1 stars 0 forks source link

Missing check in the updateValset function #123

Open code423n4 opened 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-L358

Vulnerability details

Impact

The updateValset function don't check that the sum of the powers of the new validators in the new valset is greater than the threshold, which can lead to unwanted behavior.

There are 2 main problems that can occur in that situation:

  1. The sum of the new validators' powers will be lower than the state_powerThreshold
  2. The sum of the new validators' power will overflow and become lower than the state_powerThreshold

The second case is less dangerous, because it won't stuck the system in every case (only in specific cases where every sum of validators' power is less than the threshold). The first case is very dangerous though. It can lead to the system becoming stuck and to all of the tokens on the cudos chain to become locked for users, because the validators won't have enough power to approve any operation - whether it is transferring tokens or updating the valset.

Proof of Concept

For the first case, consider the current validators set containing 100 validators with each ones power being equal to 10, and the threshold is 900 (91+ validators are needed for approvement). Now the updateValset function is being called with 100 validators with each ones power being equal to 1. This will lead to a state where no matter how much validators have signed a message, the sum of the powers won't pass the threshold and the action won't be able to be executed. This will cause all the tokens in the cudos blockchain become locked, and will DoS all the actions of the gravity contract - including updating the valset.

For the second case, consider the new validators set will have 128 validators, each validator's power is equal to 2**249 and _powerThreshold = 2**256 - 1. In this case the system will be stuck too, because every sum of validators' power won't pass the threshold.

Tools Used

Remix and VS Code

Recommended Mitigation Steps

Add a check in the updateValset to assure that the sum of the new powers is greater than the threshold.

V-Staykov commented 2 years ago

This check is done on the Gravity module side and since the message is also signed there by the validators, we can consider it to be always as per the module, unless there are malicious validators with more voting power than the threshold.

If the message is considered correct this means that the values of the power are normalized which is in the core of the power threshold calculation. When they are normalized this means that the sum of the validator set will always equal 100% of the power which is more than the threshold.

Here is a link to the power normalization in the Gravity module side: https://github.com/code-423n4/2022-05-cudos/blob/main/module/x/gravity/keeper/keeper_valset.go#L206

albertchon commented 2 years ago

Agreed with @V-Staykov - this would only fail if 2/3+ of the validator stake weight were controlled by malicious validators at which point all bets are off