SKALE contest - defsec's results

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

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