setCollectionCosts lacks input validation, risking incorrect price calculation and exploitation.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L157
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L44-L53
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L161
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L157

Vulnerability details

Impact

Incorrect rate parameters in setCollectionCosts could lead to unexpected prices. The rate parameter in setCollectionCosts is used to calculate minting price but lacks input validation.

This could allow setting unexpected rate values that break the price calculation. This allows setting any _rate value:

_rate = 0
_rate = 1000000000 

Breaking the price calculation and charging incorrect prices.

Bug Description

My observation about the lack of input validation on the _rate parameter in the setCollectionCosts function. This could lead to unexpected behavior when calculating minting prices.

Here is my assessment:

• The _rate value passed to setCollectionCosts is stored in the collectionPhases struct and later used to calculate prices.

• However, there is no validation on _rate before storing it.

• This allows setting any arbitrary _rate value, which could break the price calculation when used.

• For example, an extremely large _rate could cause prices to decrease too rapidly or even go negative.

• By setting an unreasonable _rate then profiting from the resulting incorrect prices.

In summary, the unrestricted _rate input allows manipulating the price calculation and could lead to unexpected user charges or other economic impacts.

I would recommend adding input validation on _rate in setCollectionCosts, such as:

require(_rate > 0 && _rate < 1000000, "Invalid rate"); 

This would prevent storing invalid _rate values that could be exploited.

Proof of Concept

The setCollectionCosts function sets the rate parameter used for minting price calculation but does not validate the input value.

MinterContract.sol#setCollectionCosts

function setCollectionCosts(

  // ...

  uint256 _rate

  // ...

The rate is stored in collectionPhases struct: MinterContract.sol#collectionPhasesDataStructure

setCollectionCosts sets the rate value: MinterContract.sol#Line 161

collectionPhases[_collectionID].rate = _rate;

struct collectionPhasesDataStructure {

  // ...

  uint256 rate;

  // ...

}

And later used to calculate price.

price = startingPrice - (rate * timePeriods) 

However, setCollectionCosts does no validation on _rate: MinterContract.sol#Line 157

// No input validation
uint256 _rate  

It's because of lack of input validation on _rate in setCollectionCosts:

// Should add validation
uint256 _rate

Potential Implications

• Allowing extremely high or low rate values would cause the price calculation to behave incorrectly.

• Users would pay unexpected prices.

• Could disrupt revenue model.

• Attacker could profit by manipulating rates.

Steps to Reproduce

1. Call setCollectionCosts with a very high rate value:

setCollectionCosts(..., rate = 10000000000, ...)

2. Call the minting function.

3. Price paid will be incorrect based on invalid rate.

4. Attacker profits from price discrepancy.

Tools Used

Manual

Mitigation

Should validate rate range. Add rate range check:

require(_rate >= 10 && _rate <= 100, "Invalid rate") 

This prevents unexpected values being used.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden specifies that the _rate variable when configuring a collection is unsanitized, however, they fail to articulate why and how this could be potentially weaponized.

Given that the _rate variable should seemingly be arbitrarily specified per collection, I consider this exhibit to have insufficient proof.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Insufficient proof