Cudos contest - ellahi's results

Low

[L-01] Unspecific Compiler Version Pragma

Impact

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

Proof of Concept

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

Recommendation

Avoid floating pragmas for non-library contracts. It is recommended to pin to a concrete compiler version.

[L-02] Test functions stil in contract.

Make sure to get rid of test functions before deployment.

Proof of Concept

CosmosToken.sol::140 => function testMakeCheckpoint(ValsetArgs memory _valsetArgs, bytes32 _gravityId) public pure {
CosmosToken.sol::144 => function testCheckValidatorSignatures(

Tools used

manual, slither

Gas

[G-01] Cache Array Length Outside of Loop.

Impact

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.

Proof of Concept

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++) {

Recommendation

Store the array’s length in a variable before the for-loop.

[G-02] Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

Recommendation

Use != 0 instead of > 0.

[G-03] Reduce the size of error messages (Long revert Strings).

Impact

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.

Proof of Concept

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."

Recommendation

Shorten the revert strings to fit in 32 bytes.

[G-04] public Functions can be external.

Impact

public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

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)
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)

Recommendation

Declare visibility of functions above as external.

[G-05] No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

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++) {

Recommendation

Remove explicit zero initialization.

[G-06] ++i costs less gas compared to i++ or i += 1

Impact

++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.

Proof of Concept

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++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-07] State variables that could be declared constant

Impact

Constant state variables should be declared constant to save gas.

Proof of Concept

CosmosToken.sol::5 => uint256 MAX_UINT = 2**256 - 1; // @audit make constant, saves gas.

Recommendation

Add the constant attributes to state variables that never change.

Tools used

c4udit, manual, slither