Cudos contest - oyc_109's results

Unspecific Compiler Version Pragma

description

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

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.

It is recommended to pin to a concrete compiler version.

findings

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

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

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

    contract C {
        function add(uint[] memory arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

    contract C {
        function add(uint[] calldata arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

findings

File: Gravity.sol
124: function manageWhitelist(
125: 		address[] memory _users,
126: 		bool _isWhitelisted
127: 		) public onlyWhitelisted {

...

144: 	function testCheckValidatorSignatures(
145: 		address[] memory _currentValidators,
146: 		uint256[] memory _currentPowers,
147: 		uint8[] memory _v,
148: 		bytes32[] memory _r,
149: 		bytes32[] memory _s,
150: 		bytes32 _theHash,
151: 		uint256 _powerThreshold
152: 	) public pure {

...

219: 	function checkValidatorSignatures(
220: 		// The current validator set and their powers
221: 		address[] memory _currentValidators,
222: 		uint256[] memory _currentPowers,
223: 		// The current validator's signatures
224: 		uint8[] memory _v,
225: 		bytes32[] memory _r,
226: 		bytes32[] memory _s,
227: 		// This is what we are checking they have signed
228: 		bytes32 _theHash,
229: 		uint256 _powerThreshold
230: 	) private pure {

...

364: function submitBatch (
365: 		// The validators that approve the batch
366: 		ValsetArgs memory _currentValset,
367: 		// These are arrays of the parts of the validators signatures
368: 		uint8[] memory _v,
369: 		bytes32[] memory _r,
370: 		bytes32[] memory _s,
371: 		// The batch of transactions
372: 		uint256[] memory _amounts,
373: 		address[] memory _destinations,
374: 		uint256[] memory _fees,
375: 		uint256 _batchNonce,
376: 		address _tokenContract,
377: 		// a block height beyond which this batch is not valid
378: 		// used to provide a fee-free timeout
379: 		uint256 _batchTimeout
380: 	) public nonReentrant {

...

479: 	function submitLogicCall(
480: 		// The validators that approve the call
481: 		ValsetArgs memory _currentValset,
482: 		// These are arrays of the parts of the validators signatures
483: 		uint8[] memory _v,
484: 		bytes32[] memory _r,
485: 		bytes32[] memory _s,
486: 		LogicCallArgs memory _args
487: 	) public nonReentrant {

...

640: 	constructor(
641: 		// A unique identifier for this gravity instance to use in signatures
642: 		bytes32 _gravityId,
643: 		// How much voting power is needed to approve operations
644: 		uint256 _powerThreshold,
645: 		// The validator set, not in valset args format since many of it's
646: 		// arguments would never be used in this case
647: 		address[] memory _validators,
648:     uint256[] memory _powers,
649: 		CudosAccessControls _cudosAccessControls
650: 	) public {

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

File: Gravity.sol
54: uint256 public state_lastValsetNonce = 0;
128: for (uint256 i = 0; i < _users.length; i++) {
231: uint256 cumulativePower = 0;
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++) {
659: uint256 cumulativePower = 0;
660: for (uint256 i = 0; i < _powers.length; i++) {

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

File: Gravity.sol
128: for (uint256 i = 0; i < _users.length; i++) { //@audit _users.length should be cached outside of for loop
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++) {

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

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

Long Revert Strings

description

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

File: Gravity.sol
240: "Validator signature does not match."
256: "Submitted validator set signatures do not have enough power."
291: "New valset nonce must be greater than the current nonce"
312: "Supplied current validators and powers do not match checkpoint."
317: "The sender of the transaction is not validated orchestrator"
386: "New batch nonce must be greater than the current nonce"
392: "Batch timeout must be greater than the current block height"
407: "Supplied current validators and powers do not match checkpoint."
418: "The sender of the transaction is not validated orchestrator"
496: "New invalidation nonce must be greater than the current nonce"
511: "Supplied current validators and powers do not match checkpoint."
517: "Malformed list of token transfers"
527: "The sender of the transaction is not validated orchestrator"

do not cache variable used only once

description

for variables only used once, changing it to inline saves gas

findings

File: Gravity.sol
636: uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this));