search

code-423n4 / 2022-02-hubble-findings

2 stars 2 forks source link

QA Report #92

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

C4-001 : PREVENT DIV BY 0

Impact - LOW

On several locations in the code precautions are taken not to divide by 0, because this will revert the code. However on some locations this isn’t done.

Oracle price is not checked. That will cause to revert on the several functions.

Proof of Concept

  1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/Oracle.sol#L34

Tools Used

None

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.

C4-002 : Single-step change of governance address is extremely risky

Impact - LOW

Single-step change of critical governance address and lack of zero address check is extremely risky. If a zero address or incorrect address (private key not available) is used accidentally, or maliciously changed by a compromised governance account then the entire governance of the protocol is locked forever or lost to an attacker. No governance changes can be made by authorized governance account and protocol will have to be redeployed. The reputation of the protocol will take a huge hit. There may be significant fund lock/loss as well.

Interestingly, this 2-step process is applied to the changing of Strategist address but not Governance address. Governance has more authority in the protocol because it can change the Strategist among other things. So this 2-step should definitely be applied to Governance as well.

Given the magnitude of the impact, i.e. permanent lock of all governance actions, potential lock/loss of funds, and the known/documented failures of wallet opsec, this risk is classified as medium severity.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/legos/Governable.sol#L20

 Tool

Code Review

Recommended Mitigation Steps

Change of the most critical protocol address i.e. governance should be timelocked and be a 2-step process: approve+claim in two different transactions, instead of a single-step change.

C4-003 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/legos/HubbleBase.sol#L49

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-004 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/AMM.sol#L93

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/Oracle.sol#L20

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/VUSD.sol#L38

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccount.sol#L121
  1. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-005 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

The protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccount.sol#L155

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccountHelper.sol#L29

Tools Used

Manual Code Review

Recommended Mitigation Steps

C4-006 : Missing zero-address check in constructors and the setter functions

Impact - LOW

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

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccountHelper.sol#L19

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/VUSD.sol#L39

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/legos/Governable.sol#L16

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/InsuranceFund.sol#L35

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccount.sol#L121

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

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

Impact - Non critical

The governor only functions 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.

There are owner functions that do not emit any events in the contracts.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccount.sol#L616

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/legos/Governable.sol#L19

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/Oracle.sol#L162

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/VUSD.sol#L74

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/AMM.sol#L722

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/AMM.sol#L737

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 admin/privileged functions that change critical parameters.

C4-008 : Deprecated safeApprove() function

Impact - LOW

Detailed description of the impact of this finding.

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccountHelper.sol#L24

Tools Used

Code Review

Recommended Mitigation Steps

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

C4-009 : The Contract Should Approve(0) first

Impact - LOW

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccountHelper.sol#L24

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-0010 : The Contract Should Approve(0) first

Impact - LOW

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/MarginAccountHelper.sol#L24

Tools Used

Code Review

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-0011 : Missing Pause Modifier On the InsuranceFunds contract

Impact - Low

In case a hack occurs or an exploit is discovered, the team should be able to pause functionality until the necessary changes are made to the system. The deposits should be paused with Pause modifier.

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/InsuranceFund.sol#L39

There is no pause mechanism has been defined.

Tools Used

Code Review

Recommended Mitigation Steps

Pause functionality on the contract would have helped secure the funds quickly.

C4-0012 : Missing Re-entrancy Guard

Impact - Non-critical

The re-entrancy guard is missing on the Eth anchor interaction. The external router interaction can cause to the re-entrancy vulnerability.

Proof of Concept

  1. Navigate to the following contract functions.
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/InsuranceFund.sol#L39

Tools Used

Code Review

Recommended Mitigation Steps

Follow the check effect interaction pattern or put re-entrancy guard.

atvanguard commented 2 years ago

Good QA report.

moose-code commented 2 years ago

Lots of good insights here :100:

JasoonS commented 2 years ago

Quick review: C4-001 - agree - low C4-002 - agree - low C4-003 - agree - non-crit C4-004 - agree - low C4-005 - agree - low C4-006 - agree - low C4-007 - would make this a Low - not having events for critical system parameter updates can cause bigger difficulties and pains in the future C4-008 - agree - low C4-009 - agree - low C4-010 - agree - low C4-011 - agree - low C4-012 - agree - non-crit