Cudos contest - WatchPug's results

Lines of code

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

Vulnerability details

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

In Gravity.sol#updateValset(), while the signatures of the current validators are verified and >= powerThreshold is checked, there is one important validation should be done: check the cumulative power of the new validator set to ensure the contract has sufficient power to actually pass a vote.

With this check missing, a malformed update with the new validator set unable to pass any vote even with the signatures of all the validators, the Gravity.sol contract will be bricked.

PoC

Given:

• _powerThreshold: 10
• _currentValset:
  • ValidatorA: 3
  • ValidatorB: 3
  • ValidatorC: 3
  • ValidatorD: 3

When the 3 validators: A, B, C voted to remove ValidatorD with updateValset(), the transcation will success and as a result the contract will be bricked and can no longer execute any transcation that requires a vote from the validators.

Recommendation

Consider adding the following code into updateValset() before L320:

// Check cumulative power to ensure the contract has sufficient power to actually pass a vote
uint256 cumulativePower = 0;
for (uint256 i = 0; i < _newValset.powers.length; i++) {
    cumulativePower = cumulativePower + _newValset.powers[i];
    if (cumulativePower > constant_powerThreshold) {
        break;
    }
}
require(
    cumulativePower > constant_powerThreshold,
    "Submitted validator set signatures do not have enough power."
);

Lines of code

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

Vulnerability details

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

function withdrawERC20(
	address _tokenAddress) 
	external {
	require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin");
	uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this));
	IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance);
}

The addresses hasAdminRole in cudosAccessControls can call withdrawERC20() and take all the locked funds in the Gravity.sol contract.

We believe this is extremely dangerous and unnecessary.

A malicious or compromised admin can run all the locked funds from the contract and cause huge loss to the users.

Recommendation

Consider removing this method.

Lines of code

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

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, sendToCosmos() assumes that the received amount is the same as the transfer amount, and uses it to emit SendToCosmosEvent event.

As a result, when bridging the token back from Cosmos, it may revert because of insufficient balance.

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

	function sendToCosmos(
		address _tokenContract,
		bytes32 _destination,
		uint256 _amount
	) public nonReentrant  {
		IERC20(_tokenContract).safeTransferFrom(msg.sender, address(this), _amount);
		state_lastEventNonce = state_lastEventNonce.add(1);
		emit SendToCosmosEvent(
			_tokenContract,
			msg.sender,
			_destination,
			_amount,
			state_lastEventNonce
		);
	}

Recommendation

Consider comparing before and after balance to get the actual transferred amount:

function sendToCosmos(
	address _tokenContract,
	bytes32 _destination,
	uint256 _amount
) public nonReentrant {
	uint256 preBalance = IERC20(_tokenContract).balanceOf(address(this));
	IERC20(_tokenContract).safeTransferFrom(msg.sender, address(this), _amount);
	uint256 postBalance = IERC20(_tokenContract).balanceOf(address(this));

	state_lastEventNonce = state_lastEventNonce.add(1);
	emit SendToCosmosEvent(
		_tokenContract,
		msg.sender,
		_destination,
		postBalance.sub(preBalance),
		state_lastEventNonce
	);
}

[L] Arithmetic operations without using SafeMath may over/underflow

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

for (uint256 i = 0; i < _currentValidators.length; i++) {
    // If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation
    // (In a valid signature, it is either 27 or 28)
    if (_v[i] != 0) {
        // Check that the current validator has signed off on the hash
        require(
            verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]),
            "Validator signature does not match."
        );

        // Sum up cumulative power
        cumulativePower = cumulativePower + _currentPowers[i];

        // Break early to avoid wasting gas
        if (cumulativePower > _powerThreshold) {
            break;
        }
    }
}

In L244, even though it's unlikely to overflow in this particular case, we still recommend using SafeMath instead.

[N] Outdated compiler version

It's a best practice to use the latest compiler version.

The specified minimum compiler version is quite old. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.

List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

[N] Unused function

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

	function manageWhitelist(
		address[] memory _users,
		bool _isWhitelisted
		) public onlyWhitelisted {
		 for (uint256 i = 0; i < _users.length; i++) {
            require(
                _users[i] != address(0),
                "User is the zero address"
            );
            whitelisted[_users[i]] = _isWhitelisted;
        }
        emit WhitelistedStatusModified(msg.sender, _users, _isWhitelisted);
	}

[N] SendToCosmosEvent._destination can be changed to string rather than bytes32 to support different chains in the future to indicate almost anything else we may

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

	event SendToCosmosEvent(
		address indexed _tokenContract,
		address indexed _sender,
		bytes32 indexed _destination,
		uint256 _amount,
		uint256 _eventNonce
	);

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[M-1] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

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

	function manageWhitelist(
		address[] memory _users,
		bool _isWhitelisted
		) public onlyWhitelisted {
		 for (uint256 i = 0; i < _users.length; i++) {
            require(
                _users[i] != address(0),
                "User is the zero address"
            );
            whitelisted[_users[i]] = _isWhitelisted;
        }
        emit WhitelistedStatusModified(msg.sender, _users, _isWhitelisted);
	}

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

		for (uint256 i = 0; i < _currentValidators.length; i++) {

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

		for (uint256 i = 0; i < _newValset.validators.length; i++) {

[M-2] Outdated versions of OpenZeppelin library

Outdated versions of OpenZeppelin library are used.

It's a best practice to use the latest version of libraries.

New versions of OpenZeppelin libraries can be more gas effeicant.

For exmaple:

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

import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

[M-3] Test functions should be removed

Unused code increase deploy codesize and add gas cost.

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

	function testCheckValidatorSignatures(
		address[] memory _currentValidators,
		uint256[] memory _currentPowers,
		uint8[] memory _v,
		bytes32[] memory _r,
		bytes32[] memory _s,
		bytes32 _theHash,
		uint256 _powerThreshold
	) public pure {
		checkValidatorSignatures(
			_currentValidators,
			_currentPowers,
			_v,
			_r,
			_s,
			_theHash,
			_powerThreshold
		);
	}

[M-4] Use short reason strings can save gas

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

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

		require(
			cumulativePower > _powerThreshold,
			"Submitted validator set signatures do not have enough power."
		);

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

		require(
			_newValset.valsetNonce > _currentValset.valsetNonce,
			"New valset nonce must be greater than the current nonce"
		);

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

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

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

		require(
			isOrchestrator(_currentValset, msg.sender),
			"The sender of the transaction is not validated orchestrator"
		);

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

			require(
				state_lastBatchNonces[_tokenContract] < _batchNonce,
				"New batch nonce must be greater than the current nonce"
			);

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

			require(
				block.number < _batchTimeout,
				"Batch timeout must be greater than the current block height"
			);

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

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

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

        require(
            isOrchestrator(_currentValset, msg.sender),
            "The sender of the transaction is not validated orchestrator"
        );

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

        require(
            state_invalidationMapping[_args.invalidationId] < _args.invalidationNonce,
            "New invalidation nonce must be greater than the current nonce"
        );

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

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

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

        require(
            isOrchestrator(_currentValset, msg.sender),
            "The sender of the transaction is not validated orchestrator"
        );

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

		require(
			cumulativePower > _powerThreshold,
			"Submitted validator set signatures do not have enough power."
		);

[S-5] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• Gravity.sol#manageWhitelist()

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

• Gravity.sol#checkValidatorSignatures()

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

• Gravity.sol#isOrchestrator()

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

• Gravity.sol#submitBatch()

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

• Gravity.sol#submitLogicCall()

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

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

• Gravity.sol#constructor()

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

[M-6] Using constants instead of local variables can save some gas

hhttps://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/CosmosToken.sol#L5

	uint256 MAX_UINT = 2**256 - 1;

[S-7] Using immutable variable can save gas

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

	// These are set once at initialization
	bytes32 public state_gravityId;
	uint256 public state_powerThreshold;

Considering that state_gravityId should not change, changing it to immutable variable instead of storage variable can save gas.

[S-8] CosmosERC20.sol use immutable for ERC20 decimals() can save gas

immutable is much cheaper in gas compared to storage.

Recommendation

Consider changing to:

pragma solidity ^0.6.6;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract CosmosERC20 is ERC20 {
	uint256 MAX_UINT = 2**256 - 1;

	uint8 private immutable decimals;

	constructor(
		address _gravityAddress,
		string memory _name,
		string memory _symbol,
		uint8 _decimals
	) public ERC20(_name, _symbol) {
		decimals =  _decimals;
		_mint(_gravityAddress, MAX_UINT);
	}

	function decimals() public view override returns (uint8) {
		return decimals;
	}
}

[M-9] Public functions not used by current contract should be external

For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external functions' parameters are not copied into memory and are instead read from calldata directly.

For example:

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

	function deployERC20(
		string memory _cosmosDenom,
		string memory _name,
		string memory _symbol,
		uint8 _decimals
	) public {
		// ...
	}

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

	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 {
		// ...
	}

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

	function submitBatch (
		// The validators that approve the batch
		ValsetArgs memory _currentValset,
		// These are arrays of the parts of the validators signatures
		uint8[] memory _v,
		bytes32[] memory _r,
		bytes32[] memory _s,
		// The batch of transactions
		uint256[] memory _amounts,
		address[] memory _destinations,
		uint256[] memory _fees,
		uint256 _batchNonce,
		address _tokenContract,
		// a block height beyond which this batch is not valid
		// used to provide a fee-free timeout
		uint256 _batchTimeout
	) public nonReentrant {
		// ...
	}

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

	function submitLogicCall(
		// The validators that approve the call
		ValsetArgs memory _currentValset,
		// These are arrays of the parts of the validators signatures
		uint8[] memory _v,
		bytes32[] memory _r,
		bytes32[] memory _s,
		LogicCallArgs memory _args
	) public nonReentrant {
		// ...
	}