CoreCollection can be reinitialized

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L78-L97

Vulnerability details

Impact

Reinitialization is possible for CoreCollection as initialize function sets initialized flag, but doesn't control for it, so the function can be rerun multiple times.

Such types of issues tend to be critical as all core variables can be reset this way, for example payableToken, which provides a way to retrieve all the contract funds.

However, setting priority to be medium as initialize is onlyOwner. A run by an external attacker this way is prohibited, but the possibility of owner initiated reset either by mistake or with a malicious intent remains with the same range of system breaking consequences.

Proof of Concept

initialize doesn't control for repetitive runs:

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L87

Recommended Mitigation Steps

Add onlyUnInitialized modifier to the initialize function:

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L46-L49

[sofianeOuafir]

This is a high severity issue and we intend to fix it. The mitigation step looks great and will be considered to fix the issue.

In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk)

[deluca-mike]

Not convinced this is a high severity issue, since erroneously changing payableToken via a re-initialization can simply be corrected by a re-re-initialization to set it back correctly. Further, as the warden mentioned, the initialize function is behind onlyOwner.

However, if it can be shown that users other than the owner can end up losing value due to the owner abusing or erroneously using initialize, then it can be promoted to High Severity.

And just as I say that, #17 points that out clearly. So, yes, agreed, this is a High Severity issue.