Cudos contest - ilan's results

L01 cumulativePower does not use safe addition and can overflow

In both Gravity.sol#L659 and Gravity.sol#L233, _currentPowers[i] is summed using the + operator and not the safe .add():

cumulativePower = cumulativePower + _currentPowers[i]

As a result, cumulativePower could overflow and require(cumulativePower > _powerThreshold); would fail.

Recommendation

use .add() for safe addition, or make sure to check every iteration that if

max_int - _currentPowers[i] < cumulativePower

then

cumulativePower = max_int

G01 Variables should be immutable

Some variables could be set immutable to save gas, in Gravity.sol

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

CudosAccessControls public cudosAccessControls;

should be immutable:

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

CudosAccessControls public immutable cudosAccessControls;

G02 avoid uneccesary require checks in checkValidatorSignatures

Gravity.sol#L231-L258 In the loop, if cumulativePower > _powerThreshold then the loop will break and require the exact same thing. If this if statement was not satisfied until the end of the loop, we know we haven't passed the threshold. I suggest this:

uint256 cumulativePower;
bool passedThreshold;

    for (uint256 i; 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) {
                passedThreshold = true;
                break;
            }
        }
    }

    // Check that there was enough power
    require(
        passedThreshold,
        "Submitted validator set signatures do not have enough power."
    );
    // Success
}

To take it a step further, we can just return; once cumulativePower > _powerThreshold and call require(false) after the loop. However this is less readable so I'll leave the choice to you.

Do the same for Gravity.sol#L657-L670

General Gas Saving Good Practices

G03 Change i++ to ++i

i++ is generally more expensive because it must increment a value and return the old value, so it may require holding two numbers in memory. ++i only uses one number in memory.

Example:

for (uint256 i = 0; i < _users.length; i++) -> for (uint256 i = 0; i < _users.length; ++i) Gravity.sol#L128

G04 Use calldata

Use calldata instead of memory for external functions where the function argument is read-only. Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

G05 No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to 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.

Example:

uint256 i = 0 -> uint256 i Gravity.sol#L128