Cudos contest - 0xkatana's results

[L-01] No ecrecover zero address check

From a solidity documentation warning https://docs.soliditylang.org/en/v0.8.13/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions

If you use ecrecover, be aware that a valid signature can be turned into a different valid signature without requiring knowledge of the corresponding private key. In the Homestead hard fork, this issue was fixed for _transaction_ signatures (see EIP-2), but the ecrecover function remained unchanged.

This is usually not a problem unless you require signatures to be unique or use them to identify items. OpenZeppelin have a ECDSA helper library that you can use as a wrapper for ecrecover without this issue.

The zero address is not checked when ecrecover is used in Gravity and the ECDSA helper library from OpenZeppelin is not used. This could lead to the zero address passing the ecrecover signature check. The same type of bug caused a $2 million bounty in Polygon code https://medium.com/immunefi/polygon-lack-of-balance-check-bugfix-postmortem-2-2m-bounty-64ec66c24c7d

Recommended Mitigation Steps

Check that the result of ecrecover is not equal to the zero address. This example is taken from Compound https://github.com/compound-finance/compound-protocol/blob/3affca87636eecd901eb43f81a4813186393905d/contracts/Governance/Comp.sol#L165-L166 require(signatory != address(0))

[L-02] Admin can rug using withdrawERC20

withdrawERC20 allows the admin to remove any ERC20 token from Gravity contract. This can lead to zero liquidity on the bridge, or an admin rugging users. While the admin may be assumed to be a trusted user, the risk remains because of the possibility of account compromise.

The relevant function is in Gravity https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L632-L638

Recommended Mitigation Steps

Prevent admin from transferring ERC20 tokens serving as bridge liquidity or collateral. If this feature is necessary, add an additional check that only allows this withdrawal to happen when emergency mode is activated

[L-03] Old vulnerable OpenZeppelin dependency version

OpenZeppelin is a common library to import. The Cudos code uses solidity 0.6.6 and version 3.1.0 of the OpenZeppelin library. The versions are outdated and have minor vulnerabilities. Some of the vulnerabilities from the OpenZeppelin release notes are listed at https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories

No vulnerabilities on OpenZeppelin 3.1.0 specifically impact the Cudos code, but because this version is so old, it may contain vulnerabilities that no one is looking for because the problems may not exist in newer OpenZeppelin versions, which are the subject of more security research.

Recommended Mitigation Steps

Use Solidity 0.7.X or 0.8.X. Update OpenZeppelin library version to 3.4 or newer. Using newest library version to have better security and lower gas costs

[G-01] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

There were many places where an int is initialized to zero https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L54 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L128 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L231 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L233 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L263 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L453 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L579 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L659

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-02] Split up require statements instead of &&

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

One example is

require(_amounts.length == _destinations.length && _amounts.length == _fees.length);

This can be improved to

require(_amounts.length == _destinations.length);
require(_amounts.length == _fees.length);

Several places had require statements with many logical "and"s. Instead, split into two to save gas https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L302-L305 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L397-L400 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L412 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L501-L504

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

[G-03] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L119 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L240 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L256 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L291 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L312 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L317 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L384 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L392 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L407 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L418 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L496 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L511 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L527 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L655 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L668

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-04] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

There are many examples of this in for loops https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L128 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L233 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L263 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L453 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L579 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L660

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-05] Use type(uint256).max

The value 2**256 - 1 is used for MAX_UINT but using type(uint256).max uses less gas than this constant value. Source https://forum.openzeppelin.com/t/using-the-maximum-integer-in-solidity/3000/13

There is one example of this where changes can save gas https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/CosmosToken.sol#L5

Recommended Mitigation Steps

Use type(uint256).max

[G-06] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

This was found in many places https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L128 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L233 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L263 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L296 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L302 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L303 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L304 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L305 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L397 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L398 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L399 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L400 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L412 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L453 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L501 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L502 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L503 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L504 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L516 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L522 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L579 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L654

Recommended Mitigation Steps

Cache the array length before the for loop

[G-07] Use newer solidity version

Solidity 0.6.6 is used in Cudos https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L1

Newer versions of solidity include gas optimizations. One example from solidity 0.8.10 is quoted from https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/#full-changelog

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

Version 0.8.13 can save more gas with Yul IR pipeline https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/#yul-ir-pipeline-production-ready

Recommended Mitigation Steps

Use a newer solidity release, version 0.8.10 or newer