search

code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

QA Report #253

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 : Missing zero-address/values check in the constructor - Low

C4-004 : transferOwnership should be two step process - Low

C4-005 : Contract should have pause/unpause functionality - Low

C4-006 : Use of Block.timestamp - Low

C4-007 : Centralization Risk - 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-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L364

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L375

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L351

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-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L364

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L375

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L351

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 : # Missing zero-address&values check

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:

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L49

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L104

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address and zero value checks.

C4-004 : # transferOwnership should be two step process

Impact

The contracts inherit OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

  1. Navigate to "https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L50".
  2. The contracts have many onlyOwner function.
  3. The contract is inherited from the Ownable which includes transferOwnership.

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

C4-005 : Contract should have pause/unpause functionality

Impact

In case a hack is occuring or an exploit is discovered, the team should be able to pause functionality until the necessary changes are made to the system.

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)

Proof of Concept

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L50

Tools Used

Code Review

Recommended Mitigation Steps

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

C4-006 : 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-04-mimo/blob/main/core/contracts/inception/InceptionVaultsCore.sol#L51

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-007 : Centralization Risk

Impact - LOW

The system is heavily relies on the owner role. Therefore, It contains centralization risk If the owner is EOA and captured.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L50

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L15

Tools Used

None

Recommended Mitigation Steps

We advise the client to carefully manage the executor accounts' private key to avoid any potential risks of being hacked. In general, we strongly recommend centralized privileges or roles in the protocol to be improved via a decentralized mechanism or smart-contract-based accounts with enhanced security practices, e.g., Multi-Signature wallets.

nneverlander commented 2 years ago

Thanks

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-06-infinity-findings/issues/167