Cudos contest - Dravee's results

Lines of code

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

Vulnerability details

Impact

Wrong amount emitted in SendToCosmosEvent event

Proof of Concept

File: Gravity.sol
595: 	function sendToCosmos(
596: 		address _tokenContract,
597: 		bytes32 _destination,
598: 		uint256 _amount
599: 	) public nonReentrant  {
600: 		IERC20(_tokenContract).safeTransferFrom(msg.sender, address(this), _amount); //@audit FoT
601: 		state_lastEventNonce = state_lastEventNonce.add(1); 
602: 		emit SendToCosmosEvent(
603: 			_tokenContract,
604: 			msg.sender,
605: 			_destination,
606: 			_amount,
607: 			state_lastEventNonce 
608: 		);
609: 	}

As arbitrary ERC20 tokens can be passed, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

Table of Contents:

• [L-01] Missing SafeMath at some places
• [N-01] Avoid floating pragmas: the version should be locked

[L-01] Missing SafeMath at some places

At some places, the risk of overflow exists but SafeMath isn't used:

solidity/contracts/Gravity.sol:
  244:     cumulativePower = cumulativePower + _currentPowers[i];
  661:    cumulativePower = cumulativePower + _powers[i];

These are the only places missing SafeMath's use. Consider using SafeMath's addition there, to be consistent.

[N-01] Avoid floating pragmas: the version should be locked

CosmosToken.sol:1:pragma solidity ^0.6.6;
Gravity.sol:1:pragma solidity ^0.6.6;

Table of Contents:

• Cheap Contract Deployment Through Clones
• Caching storage values in memory
• Splitting require() statements that use && saves gas
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Public functions to external
• onlyWhitelisted: A modifier used only once should be inlined
• No need to explicitly initialize variables with default values
• Reduce the size of error messages (Long revert Strings)

Cheap Contract Deployment Through Clones

See @audit tag:

File: Gravity.sol
611:  function deployERC20(
612:   string memory _cosmosDenom,
613:   string memory _name,
614:   string memory _symbol,
615:   uint8 _decimals
616:  ) public {
617:   // Deploy an ERC20 with entire supply granted to Gravity.sol
618:   CosmosERC20 erc20 = new CosmosERC20(address(this), _name, _symbol, _decimals); //@audit gas: deployment can cost less through clones

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

I suggest applying a similar pattern.

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

solidity/contracts/Gravity.sol:
  311:    makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint,  //@audit gas SLOAD 1(state_gravityId)
  321:   bytes32 newCheckpoint = makeCheckpoint(_newValset, state_gravityId); //@audit gas SLOAD 2(state_gravityId)
  349:   state_lastEventNonce = state_lastEventNonce.add(1); //@audit gas SLOAD 1(state_lastEventNonce), should cache state_lastEventNonce.add(1)
  352:    state_lastEventNonce, //@audit gas SLOAD 2(state_lastEventNonce), should use suggested cache
  406:     makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, //@audit gas SLOAD 1(state_gravityId)
  431:       state_gravityId, //@audit gas SLOAD 2(state_gravityId)
  465:    state_lastEventNonce = state_lastEventNonce.add(1); //@audit gas SLOAD 1(state_lastEventNonce), should cache state_lastEventNonce.add(1)
  466:    emit TransactionBatchExecutedEvent(_batchNonce, _tokenContract, state_lastEventNonce); //@audit gas SLOAD 2(state_lastEventNonce), should use suggested cache
  510:     makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, //@audit gas SLOAD 1(state_gravityId)
  533:     state_gravityId, //@audit gas SLOAD 2(state_gravityId)
  585:    state_lastEventNonce = state_lastEventNonce.add(1); //@audit gas SLOAD 1(state_lastEventNonce), should cache state_lastEventNonce.add(1)
  590:     state_lastEventNonce //@audit gas SLOAD 2(state_lastEventNonce), should use suggested cache
  601:   state_lastEventNonce = state_lastEventNonce.add(1); //@audit gas SLOAD 1(state_lastEventNonce), should cache state_lastEventNonce.add(1)
  607:    state_lastEventNonce //@audit gas SLOAD 2(state_lastEventNonce), should use suggested cache
  621:   state_lastEventNonce = state_lastEventNonce.add(1); //@audit gas SLOAD 1(state_lastEventNonce), should cache state_lastEventNonce.add(1)
  628:    state_lastEventNonce //@audit gas SLOAD 2(state_lastEventNonce), should use suggested cache

Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:

solidity/contracts/Gravity.sol:
  301    require(
  302:    _currentValset.validators.length == _currentValset.powers.length &&
  303:     _currentValset.validators.length == _v.length &&
  304:     _currentValset.validators.length == _r.length &&
  305      _currentValset.validators.length == _s.length,
  306     "Malformed current validator set"
  307    );

  396     require(
  397:     _currentValset.validators.length == _currentValset.powers.length &&
  398:      _currentValset.validators.length == _v.length &&
  399:      _currentValset.validators.length == _r.length &&
  400       _currentValset.validators.length == _s.length,
  401      "Malformed current validator set"
  402     );

  411     require(
  412:     _amounts.length == _destinations.length && _amounts.length == _fees.length,
  413      "Malformed batch of transactions"
  414     );

  500     require(
  501:     _currentValset.validators.length == _currentValset.powers.length &&
  502:      _currentValset.validators.length == _v.length &&
  503:      _currentValset.validators.length == _r.length &&
  504       _currentValset.validators.length == _s.length,
  505      "Malformed current validator set"
  506     );

An array's length should be cached to save gas in for-loops

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.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

solidity/contracts/Gravity.sol:
  128:    for (uint256 i = 0; i < _users.length; i++) {
  233:   for (uint256 i = 0; i < _currentValidators.length; i++) {
  263:   for (uint256 i = 0; i < _newValset.validators.length; i++) {
  453:     for (uint256 i = 0; i < _amounts.length; i++) {
  568:   for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
  579:   for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
  660:   for (uint256 i = 0; i < _powers.length; i++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

The same is also true for i--.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

Gravity.sol:128:   for (uint256 i = 0; i < _users.length; i++) {
Gravity.sol:233:  for (uint256 i = 0; i < _currentValidators.length; i++) {
Gravity.sol:263:  for (uint256 i = 0; i < _newValset.validators.length; i++) {
Gravity.sol:453:    for (uint256 i = 0; i < _amounts.length; i++) {
Gravity.sol:568:  for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
Gravity.sol:579:  for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
Gravity.sol:660:  for (uint256 i = 0; i < _powers.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Public functions to external

The following functions could be set external to save gas and improve code quality (extracted from Slither). An external call cost is less expensive than one of a public function.

manageWhitelist(address[],bool) should be declared external:
 - Gravity.manageWhitelist(address[],bool) (contracts/Gravity.sol#124-136)
testMakeCheckpoint(ValsetArgs,bytes32) should be declared external:
 - Gravity.testMakeCheckpoint(ValsetArgs,bytes32) (contracts/Gravity.sol#140-142)
testCheckValidatorSignatures(address[],uint256[],uint8[],bytes32[],bytes32[],bytes32,uint256) should be declared external:
 - Gravity.testCheckValidatorSignatures(address[],uint256[],uint8[],bytes32[],bytes32[],bytes32,uint256) (contracts/Gravity.sol#144-162)
lastBatchNonce(address) should be declared external:
 - Gravity.lastBatchNonce(address) (contracts/Gravity.sol#166-168)
lastLogicCallNonce(bytes32) should be declared external:
 - Gravity.lastLogicCallNonce(bytes32) (contracts/Gravity.sol#170-172)
updateValset(ValsetArgs,ValsetArgs,uint8[],bytes32[],bytes32[]) should be declared external:
 - Gravity.updateValset(ValsetArgs,ValsetArgs,uint8[],bytes32[],bytes32[]) (contracts/Gravity.sol#276-358)
submitBatch(ValsetArgs,uint8[],bytes32[],bytes32[],uint256[],address[],uint256[],uint256,address,uint256) should be declared external:
 - Gravity.submitBatch(ValsetArgs,uint8[],bytes32[],bytes32[],uint256[],address[],uint256[],uint256,address,uint256) (contracts/Gravity.sol#364-468)
submitLogicCall(ValsetArgs,uint8[],bytes32[],bytes32[],LogicCallArgs) should be declared external:
 - Gravity.submitLogicCall(ValsetArgs,uint8[],bytes32[],bytes32[],LogicCallArgs) (contracts/Gravity.sol#479-593)
sendToCosmos(address,bytes32,uint256) should be declared external:
 - Gravity.sendToCosmos(address,bytes32,uint256) (contracts/Gravity.sol#595-609)
deployERC20(string,string,string,uint8) should be declared external:
 - Gravity.deployERC20(string,string,string,uint8) (contracts/Gravity.sol#611-630)

onlyWhitelisted: A modifier used only once should be inlined

Affected code:

solidity/contracts/Gravity.sol:
  116:  modifier onlyWhitelisted() {
  127:   ) public onlyWhitelisted {

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

Gravity.sol:54: uint256 public state_lastValsetNonce = 0;
Gravity.sol:128:   for (uint256 i = 0; i < _users.length; i++) {
Gravity.sol:231:  uint256 cumulativePower = 0;
Gravity.sol:233:  for (uint256 i = 0; i < _currentValidators.length; i++) {
Gravity.sol:263:  for (uint256 i = 0; i < _newValset.validators.length; i++) {
Gravity.sol:453:    for (uint256 i = 0; i < _amounts.length; i++) {
Gravity.sol:568:  for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
Gravity.sol:579:  for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
Gravity.sol:659:  uint256 cumulativePower = 0;
Gravity.sol:660:  for (uint256 i = 0; i < _powers.length; i++) {

I suggest removing explicit initializations for default values.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

Gravity.sol:119:            "The caller is not whitelisted for this operation"
Gravity.sol:240:     "Validator signature does not match."
Gravity.sol:256:   "Submitted validator set signatures do not have enough power."
Gravity.sol:291:   "New valset nonce must be greater than the current nonce"
Gravity.sol:312:   "Supplied current validators and powers do not match checkpoint."
Gravity.sol:317:   "The sender of the transaction is not validated orchestrator"
Gravity.sol:386:    "New batch nonce must be greater than the current nonce"
Gravity.sol:392:    "Batch timeout must be greater than the current block height"
Gravity.sol:407:    "Supplied current validators and powers do not match checkpoint."
Gravity.sol:418:    "The sender of the transaction is not validated orchestrator"
Gravity.sol:496:    "New invalidation nonce must be greater than the current nonce"
Gravity.sol:511:    "Supplied current validators and powers do not match checkpoint."
Gravity.sol:517:    "Malformed list of token transfers"
Gravity.sol:527:    "The sender of the transaction is not validated orchestrator"
Gravity.sol:655:  require(address(_cudosAccessControls) != address(0), "Access control contract address is incorrect");
Gravity.sol:668:   "Submitted validator set signatures do not have enough power." 

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.