It's not possible to remove previously added token

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L1

Vulnerability details

Impact

Protocol implements function configureToken(), responsible for adding new tokens to configuredTokenContracts array:

File: LockManager.sol

 configuredTokenContracts.push(_tokenContract);

However, there's no functionality which allows to remove that token from configuredTokenContracts.

If there were too many elements in configuredTokenContracts, protocol will finally start to revert with out of gas error (since multiple of functions, e.g., setLockDuration(), getLocked(), getLockedWeightedValue() iterate over configuredTokenContracts array).

This is crucial to make sure that there's a way to remove old, unused, or added by misteka tokens from configureTokenContracts.

Even though function configureToken() can be called by admin only - the protocol documentation explicitly states that there's no trusted roles:

Protocol explicitly mentions that no roles are trusted:

File: README.md

All trusted roles in the protocol

N/A

This is a reason I set the severity of this issue as Medium, instead of QA.

Proof of Concept

Protocol does not implement any function which allows to remove elements from configuredTokenContracts.

File: LockManager.sol

function configureToken(
        address _tokenContract,
        ConfiguredToken memory _tokenData
    ) external onlyAdmin {
        if (_tokenData.nftCost == 0) revert NFTCostInvalidError();
        if (configuredTokens[_tokenContract].nftCost == 0) {
            // new token
            configuredTokenContracts.push(_tokenContract);

Every call of configureToken() increases the size of configuredTokenContracts array. This array keeps getting bigger and bigger - and there's no way to remove elements from it.

When this array becomes significantly big, the protocol will break, because multiple of functions will start revert with out of gas error:

• setLockDuration()

File: LockManager.sol

        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {

Calling setLockDuration() will revert with out of gas error, thus it won't be possible to set lock durations.

• getLocked(), getLockedWeightedValue()

File: LockManager.sol

        uint256 configuredTokensLength = configuredTokenContracts.length;
        LockedTokenWithMetadata[]
            memory tmpLockedTokens = new LockedTokenWithMetadata[](
                configuredTokensLength
            );
        for (uint256 i; i < configuredTokensLength; i++) {

File: LockManager.sol

        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {

Calling getLocked() and getLockedWeightedValue() will revert with out of gas error, thus it won't be possible to list locked tokens for each player and locked weight value for each player.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement additional function which allows to remove token from configuredTokenContracts.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid