search

code-423n4 / 2022-05-cudos-findings

1 stars 0 forks source link

QA Report #149

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

ISSUE LIST

C4-001 : Missing events for only functions that change critical parameters - Non Critical

C4-002 : Critical changes should use two-step procedure - Non Critical

C4-003 : Pragma Version - Non Critical

C4-004 : Missing zero-address/values check in the constructor - Low

C4-005 : Sum of validator powers should always be no less than the threshold - Non Critical

C4-006 : Front-running In The reward mechanism - LOW

C4-007 : Direct usage of ecrecover allows signature malleability - LOW

C4-008 : SafeMath library is not always used in Gravity - LOW

ISSUES

C4-001 : Missing events for only functions that change critical parameters

Impact - Non critical

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.

Proof of Concept

  1. Navigate to the following contract.
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/)

Tools Used

None

Recommended Mitigation Steps

Add events to all functions that change critical parameters.

C4-002 : Critical changes should use two-step procedure

Impact - NON CRITICAL

The critical procedures should be two step process.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L9

Tools Used

Code Review

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

C4-003 : # Pragma Version

Impact

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.

Proof of Concept

https://swcregistry.io/docs/SWC-103

All Contracts

Tools Used

Manual code review

Recommended Mitigation Steps

Lock the pragma version: delete pragma solidity 0.8.10 in favor of pragma solidity 0.8.10.

C4-004 : # Missing zero-address&values check in the constructor

Impact

Missing checks for zero-addresses&values may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

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.

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address and zero value checks.

C4-005 : Sum of validator powers should always be no less than the threshold

Impact - NON-CRITICAL

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.

Tools Used

Code Review

Recommended Mitigation Steps

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.

C4-006 : Front-running In The reward mechanism

Impact - LOW

Impact

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.

Tools Used

Code Review

Code Location

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L364

C4-007 : Direct usage of ecrecover allows signature malleability

Impact

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

Proof of Concept

https://swcregistry.io/docs/SWC-117

https://swcregistry.io/docs/SWC-121

Tools Used

Code Review

Recommended Mitigation Steps

Use the recover function from OpenZeppelin's ECDSA library for signature verification.

C4-008 : SafeMath library is not always used in Gravity

Impact

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.

Proof of Concept

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider using the SafeMath library functions in the referenced lines of code.

kstoykov commented 2 years ago

C4-006: There is check that ensures that only the validators (orchestrators) could send the transaction and therefore only they could get the reward.

albertchon commented 2 years ago

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