collection admin can still change delegation Address by calling setCollectionCosts()

[thebrittfactor]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/MinterContract.sol#L164

Vulnerability details

Impact

Function updateDelegationCollection() which update allowlist mint delegation collection prevents collection admin and any other actors from perfoming this except global and Function Admin.

However, collection admin can still change delegation Address by calling setCollectionCosts()

Proof Of Concept

setCollectionCosts():

function setCollectionCosts(uint256 _collectionID, uint256
_collectionMintCost, uint256 _collectionEndMintCost, uint256 _rate, uint256
_timePeriod, uint8 _salesOption, address _delAddress) public
CollectionAdminRequired(_collectionID, this.setCollectionCosts.selector) {
        require(gencore.retrievewereDataAdded(_collectionID) == true, "Add
data");
        collectionPhases[_collectionID].collectionMintCost =
_collectionMintCost;
        collectionPhases[_collectionID].collectionEndMintCost =
_collectionEndMintCost;
        collectionPhases[_collectionID].rate = _rate;
        collectionPhases[_collectionID].timePeriod = _timePeriod;
        collectionPhases[_collectionID].salesOption = _salesOption;
        collectionPhases[_collectionID].delAddress = _delAddress; // @audit
        setMintingCosts[_collectionID] = true;
    }

As you can see, this gives collection admin access to change delegation Address as well

Tools Used

Visual Studio Code

Recommended Mitigation Steps

update changing delegation address to be only accessed by the global and Function Admin with only updateDelegationCollection()

function setCollectionCosts(uint256 _collectionID, uint256
_collectionMintCost, uint256 _collectionEndMintCost, uint256 _rate, uint256
_timePeriod, uint8 _salesOption, address _delAddress) public
CollectionAdminRequired(_collectionID, this.setCollectionCosts.selector) {
        require(gencore.retrievewereDataAdded(_collectionID) == true, "Add
data");
        collectionPhases[_collectionID].collectionMintCost =
_collectionMintCost;
        collectionPhases[_collectionID].collectionEndMintCost =
_collectionEndMintCost;
        collectionPhases[_collectionID].rate = _rate;
        collectionPhases[_collectionID].timePeriod = _timePeriod;
        collectionPhases[_collectionID].salesOption = _salesOption;
-       collectionPhases[_collectionID].delAddress = _delAddress;
        setMintingCosts[_collectionID] = true;
    }

Assessed type

Access Control

[thebrittfactor]

For transparency, due to submission issues, the warden provided this submission prior to audit close.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

This is not an issue its the intended design. As we can have multiple allowlist minting phases each one can have its own address where the NFTDelegation contract can check the delegation status between the allowlist address (delegator) and msg.sender.

[alex-ppg]

The Warden specifies that the delegation address of a collection ID can be mutated by two different ACL members; the collection administrator as well as the "global" function administrator of MinterContract::updateDelegationCollection.

I deem this to be a non-issue given that one permits the administrator of the collection to mutate their own collection's delegation whilst the other permits the global function administrator to mutate the delegation of any collection that conforms to expected ACL layouts.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid