Cudos contest - dirk_y's results

Lines of code

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

Vulnerability details

Impact

When batches are submitted by validators in the current valset, they are checked for validity based on signatures and cumulative powers. Each validator in the valset has an associated power which can give certain validators more voting power than others. For a batch to be valid, the cumulative power of all "YES" signatures needs to be greater than _powerThreshold, which is set once during contract initialisation in the constructor.

In the constructor, there is a check for cumulative power here. However, when updating a valset, there is no check for the cumulative power of the new valset. Therefore it is theoretically possible to submit a valset that doesn't have enough cumulative power to succeed in any transactions, and then the bridge will be locked (since the valset can't be updated either).

Proof of Concept

In updateValset there is no logic for checking the cumulative power of the new valset. See below for recommended actions to resolve this.

Tools Used

VSCode

Recommended Mitigation Steps

A check for cumulative power of the new valset should be added to updateValset:

		uint256 cumulativePower = 0;
		for (uint256 i = 0; i < _newValset.powers.length; i++) {
			cumulativePower = cumulativePower + _newValset.powers[i];
			if (cumulativePower > _powerThreshold) {
				break;
			}
		}
		require(
			cumulativePower > _powerThreshold,
			"Submitted validator set signatures do not have enough power."
		);

Lines of code

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

Vulnerability details

Impact

Currently it is possible for the admin to pull all tokens belonging to the Gravity bridge. In normal circumstances this is probably fine, but if the admin account were compromised this would lead to the bridge being drained of locked funds. Furthermore, a contract that is liable to potential rugpulls is usually not trusted by users following multiple high profile rugpulls of other projects in the past.

Proof of Concept

The withdrawERC20 method here gives the admin the ability to send the full balance of the Gravity.sol contract of any token to themselves.

Tools Used

VSCode

Recommended Mitigation Steps

I assume this method exists in case the contract has to be updated and funds moved across to a new implementation. Thus, I see two potential solutions:

1. Set up a DAO that votes on transfers of ERC20 tokens by the admin
2. Remove the method altogether and utilise an upgradeable proxy pattern

Low severity findings

During my audit of this project I found 5 low priority findings in the Gravity.sol contract that I think are worth mentioning:

1. Any whitelisted address can remove all other whitelisted addresses. This is in the manageWhitelist method here. This pattern doesn't make sense to me since you only need 1 bad actor in the whitelist for this situation to occur. The offending address can then frontrun any attempts from the admin to remove them from the whitelist by adding new addresses to the whitelist. I decided not to mark this as medium severity since I can't see the whitelist being used anywhere yet. I would update this method to only be callable by the admin.

2. The comment strings explaining submitBatch and updateValset say "anyone can call this function", but in reality the msg.sender has to be an orchestrator (i.e. one of the existing validators in the valset)

3. makeCheckpoint has the following comment string:

// The format of the checkpoint is:
// h(gravityId, "checkpoint", valsetNonce, validators[], powers[])

This should be updated to:

// h(gravityId, "checkpoint", valsetNonce, validators[], powers[], rewardAmount, rewardToken)

4. There is no check in checkValidatorSignatures to confirm whether or not the array of validator powers is decreasing. The validators should be passed in as arguments in decreasing power order to optimise gas. If you wanted to enforce this you could add a check as you loop through the array to verify that the current power is less than the previous power (in the array). This might use more gas than the arrays being out of order though, depending on the length of the arrays.

5. Test fixtures are still there and should be removed (seems obvious but easily forgotten)