Platform: Code4rena
Start Date: 03/05/2022
Pot Size: $75,000 USDC
Total HM: 6
Participants: 55
Period: 7 days
Judge: Albert Chon
Total Solo HM: 2
Id: 116
League: COSMOS
Rank: 1/55
Findings: 4
Award: $13,850.66
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: defsec
12969.6852 USDC - $12,969.69
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L175
In case a hack is occuring or an exploit is discovered, the team (or validators in this case) should be able to pause functionality until the necessary changes are made to the system. Additionally, the gravity.sol contract should be manged by proxy so that upgrades can be made by the validators.
Because an attack would probably span a number of blocks, a method for pausing the contract would be able to interrupt any such attack if discovered.
To use a thorchain example again, the team behind thorchain noticed an attack was going to occur well before the system transferred funds to the hacker. However, they were not able to shut the system down fast enough. (According to the incidence report here: https://github.com/HalbornSecurity/PublicReports/blob/master/Incident%20Reports/Thorchain_Incident_Analysis_July_23_2021.pdf)
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L175
Code Review
Pause functionality on the contract would have helped secure the funds quickly.
#0 - V-Staykov
2022-05-17T07:55:23Z
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L595
The sendToCosmos function of Gravity transfers _amount of _tokenContract from the sender using the function transferFrom. If the transferred token is a transfer-on-fee/deflationary token, the actually received amount could be less than _amount. However, since _amount is passed as a parameter of the SendToCosmosEvent event, the Cosmos side will think more tokens are locked on the Ethereum side.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L595
Code Review
Consider getting the received amount by calculating the difference of token balance (using balanceOf) before and after the transferFrom.
#0 - mlukanova
2022-05-10T14:49:08Z
Duplicate of #3
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, GermanKuber, GimelSec, Hawkeye, JC, MaratCerby, WatchPug, Waze, broccolirob, cccz, ch13fd357r0y3r, cryptphi, danb, defsec, delfin454000, dipp, dirk_y, ellahi, gzeon, hake, hubble, ilan, jah, jayjonah8, kebabsec, kirk-baird, m9800, orion, oyc_109, robee, shenwilly, simon135, sorrynotsorry
116.1353 USDC - $116.14
The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L632
See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
None
Add events to all functions that change critical parameters.
The critical procedures should be two step process.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L9
Code Review
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
In the contracts, there are multiple version of pragmas are used. The contract is using pragma 0.6.6. The contracts should be deployed with the consistent pragma.
https://swcregistry.io/docs/SWC-103
All Contracts
Manual code review
Lock the pragma version: delete pragma solidity 0.8.10 in favor of pragma solidity 0.8.10.
Missing checks for zero-addresses&values may lead to infunctional protocol, if the variable addresses are updated incorrectly.
There are a few validations that could be added to the system: the constructor could check that _gravityId is not empty. state_powerThreshold should always be greater than 0, otherwise, anyone will be available to execute actions.
Code Review
Consider adding zero-address and zero value checks.
The code does not enforce that the sum of validator powers is no less than the threshold. It is possible that even when all validators sign the message their total power is not enough to confirm it.
Code Review
While this may increase the gas usage I advise you to sum the total power and check that it can reach the threshold when setting or updating validators and powers.
Once the function submitBatch lands in the mempool, anyone can copy the calldata arguments and front run the call, taking away the fees. This means that the user's tx will revert, paying for the gas. The issue here is that if the protocol relies on 'regular' users to submit transactions on the chain, then the incentive mechanism is broken.
The above problem means that only private ethereum transactions (i.e., using flashbots, taichi or perhaps eden) will make the reward mechanism work. Consider changing the submitBatch function where msg.sender can also be signed by the validators, preventing such front running issues.
Code Review
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L364
The verifySig function of Gravity calls the Solidity ecrecover function directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.
Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice (and so is checking _signer != address(0), where address(0) means an invalid signature).
https://swcregistry.io/docs/SWC-117
https://swcregistry.io/docs/SWC-121
Code Review
Use the recover function from OpenZeppelin's ECDSA library for signature verification.
SafeMath library functions are not always used in the Gravity contract's arithmetic operations, which could cause integer underflow/overflows. Using SafeMath is considered a best practice that could completely prevent underflow/overflows and increase code consistency.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L661
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L244
Code Review
Consider using the SafeMath library functions in the referenced lines of code.
#0 - kstoykov
2022-05-10T15:13:53Z
C4-006: There is check that ensures that only the validators (orchestrators) could send the transaction and therefore only they could get the reward.
#1 - albertchon
2022-08-30T06:05:39Z
Furthermore, frontrunning the reward mechanism isn't necessarily even bad, as it promotes bridge liveness. But indeed, not an issue due to the check referenced when submitting batches (require(isOrchestrator(_currentValset, msg.sender), "The sender of the transaction is not validated orchestrator")).
🌟 Selected for report: GermanKuber
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, AlleyCat, CertoraInc, Dravee, Funen, GimelSec, IllIllI, JC, MaratCerby, WatchPug, Waze, defsec, delfin454000, ellahi, gzeon, hake, hansfriese, ilan, jonatascm, nahnah, oyc_109, peritoflores, rfa, robee, simon135, slywaters, sorrynotsorry
262.36 USDC - $262.36
Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been 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 are here:
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L494 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L525 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L655
Manual Review
Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
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#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L660 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
None
Consider applying unchecked arithmetic where overflow/underflow is not possible. Example can be seen from below.
Unchecked{i++};
Since _amount can be 0. Checking if (_amount != 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L636
All Contracts
None
Consider checking amount != 0.
When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.
Example: uint x = 0 costs more gas than uint x without having any different functionality.
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#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L660 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
Code Review
uint x = 0 costs more gas than uint x without having any different functionality.
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
All Contracts
Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/
Solidity 0.8.13 has some improvements too but not well tested.
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
All Contracts
None
Consider to upgrade pragma to at least 0.8.10.
++i is more gas efficient than i++ in loops forwarding.
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#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L660 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
Code Review
It is recommend to use unchecked{++i} and change i declaration to uint256.
Using double require instead of operator && can save more gas.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L302
Code Review
Example
using &&: function check(uint x)public view{ require(x == 0 && x < 1 ); } // gas cost 21630 using double require: require(x == 0 ); require( x < 1); } } // gas cost 21622
Strict inequalities add a check of non equality which costs around 3 gas.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L662
Code Review
Use >= or <= instead of > and < when possible.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source Custom Errors in Solidity:
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Instances include:
All require Statements
Code Review
Recommended to replace revert strings with custom errors.
A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.
While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.
Contracts
None
A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.
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.
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#L568 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L660 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
None
Consider to cache array length.
Solidity 0.6.5 introduced immutable as a major feature. It allows setting contract-level variables at construction time which gets stored in code rather than storage.
Consider the following generic example:
contract C { /// The owner is set during contruction time, and never changed afterwards. address public owner = msg.sender; }
In the above example, each call to the function owner() reads from storage, using a sload. After EIP-2929, this costs 2100 gas cold or 100 gas warm. However, the following snippet is more gas efficient:
contract C { /// The owner is set during contruction time, and never changed afterwards. address public immutable owner = msg.sender; }
In the above example, each storage read of the owner state variable is replaced by the instruction push32 value, where value is set during contract construction time. Unlike the last example, this costs only 3 gas.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L60 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L61
None
Consider using immutable variable.
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.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L640 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L219
None
Some parameters in examples given above are later hashed. It may be beneficial for those parameters to be in memory rather than calldata.
#0 - V-Staykov
2022-05-10T12:23:48Z
This seems as particularly high quality, I'm just not sure what C4-010 refers to, since code is not mentioned and I can't seem to find it in the code at first glance.