search

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

0 stars 0 forks source link

QA Report #67

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

C4-001 : 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/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L59

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/SkaleManagerClient.sol#L54
  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-002 : Initializer reentrancy may lead to double initialization

Impact - LOW

Initializer functions that are invoked separate from contract creation (the most prominent example being minimal proxies) may be reentered if they make an untrusted non-view external call.

Once an initializer has finished running it can never be re-executed. However, an exception put in place to support multiple inheritance made reentrancy possible in the scenario described above, breaking the expectation that there is a single execution.

Note that upgradeable proxies are commonly initialized together with contract creation, where reentrancy is not feasible, so the impact of this issue is believed to be minor.

Proof of Concept

  1. Go to contracts directory.

  2. On the package.json, openzeppelin 4.3.2 is defined.

https://github.com/skalenetwork/ima-c4-audit/blob/main/package.json

Tools Used

None

Recommended Mitigation Steps

Avoid untrusted external calls during initialization.

A fix is included in the version v4.4.1 of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable.

Reference

https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-9c22-pwxw-p6hx

C4-003 : ERC1155Supply vulnerability in OpenZeppelin Contracts

Impact - LOW

When ERC1155 tokens are minted, a callback is invoked on the receiver of those tokens, as required by the spec. When including the ERC1155Supply extension, total supply is not updated until after the callback, thus during the callback the reported total supply is lower than the real number of tokens in circulation.

If a system relies on accurately reported supply, an attacker may be able to mint tokens and invoke that system after receiving the token balance but before the supply is updated.

Proof of Concept

  1. Go to contracts directory.

  2. On the package.json, openzeppelin 4.3.2 is defined.

https://github.com/skalenetwork/ima-c4-audit/blob/main/package.json

Tools Used

None

Recommended Mitigation Steps

A fix is included in version 4.3.3 of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable.

Reference

https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-wmpv-c2jp-j2xg

C4-004 : Missing zero-address check in the setter functions and initialize 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 contracts.
https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/CommunityLocker.sol#L253

Tools Used

Code Review

Recommended Mitigation Steps

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

DimaStebaev commented 2 years ago

C4-001 and C4-002 is described in #2 C4-002: only SKALE chain owner are able to deploy smart contracts on it's SKALE chain and this actor is assumed as trusted.

GalloDaSballo commented 2 years ago

C4-001 : Front-runnable Initializers

Agree with finding in lack of any mitigation am marking this valid. For the sponsor, this is how you can deploy and set data in one tx: https://github.com/Badger-Finance/badger-strategy-mix-v1/blob/c97eda8cb60d0dcfd62be956a2aab4c86353de65/contracts/proxy/AdminUpgradeabilityProxy.sol#L225

C4-002 : Initializer reentrancy may lead to double initialization

Finding is valid, and mitigation is to upgrade to newer OZ code

C4-003 : ERC1155Supply vulnerability in OpenZeppelin Contracts

Valid

C4-004 : Missing zero-address check in the setter functions and initialize functions

Agree

e.g. -> Vulnerability in OZ -> See disclosure -> Upgrade to xyz

GalloDaSballo commented 2 years ago

Making this the winner of QA Reports, mostly because it offers some unique findings in a proper QA Format.

In terms of Findings Kirk-Baird is basically there, I ended up making this report win as it was submitted as QA rather than an aggregate of downgraded findings.

That said I believe this report could have had a couple extra findings to make it truly a winner.

C4-001: Front-runnable Initializers -> Low

C4-002 : Initializer reentrancy may lead to double initialization -> Low

C4-003 : ERC1155Supply vulnerability in OpenZeppelin Contracts -> Low

C4-004 : Missing zero-address check in the setter functions and initialize functions -> Low