Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CoreCollection.initialize() Can Be Called Multiple Times #36

Closed
code423n4 opened this issue Mar 31, 2022 · 1 comment
Closed

CoreCollection.initialize() Can Be Called Multiple Times #36

code423n4 opened this issue Mar 31, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The function CoreCollection.initialize() may be called multiple times. This is a function only intended to be called once during startup.

As a result the owner can change variables such as mintFee (which could be used to catch unaware users to transfer more tokens than they intended during a mintTokencall),_maxSupply, payableToken, isForSaleandsplitFactorymay be changed in addition to the_name, _symbolandBaseUri`.

Proof of Concept

There are no restrictions to this function being called multiple times.

    function initialize(
        string memory _collectionName,
        string memory _collectionSymbol,
        string memory _collectionURI,
        uint256 _maxSupply,
        uint256 _mintFee,
        address _payableToken,
        bool _isForSale,
        address _splitFactory
    ) external onlyOwner onlyValidSupply(_maxSupply) {
        _name = _collectionName;
        _symbol = _collectionSymbol;
        _baseUri = _collectionURI;
        maxSupply = _maxSupply;
        mintFee = _mintFee;
        payableToken = IERC20(_payableToken);
        isForSale = _isForSale;
        splitFactory = _splitFactory;
        initialized = true;
    }

Recommended Mitigation Steps

This issue may be mitigated by calling require(!initialized); at the beginning of initialize().

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 31, 2022
code423n4 added a commit that referenced this issue Mar 31, 2022
@sofianeOuafir
Copy link
Collaborator

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

duplicate of #4

@sofianeOuafir sofianeOuafir added duplicate This issue or pull request already exists disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Apr 14, 2022
@deluca-mike deluca-mike added 3 (High Risk) Assets can be stolen/lost/compromised directly and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants