0xC - Negative `challengePeriod` can be assigned in the constructor of `TelcoinDistributor` contract

[sherlock-admin]

0xC

high

Negative challengePeriod can be assigned in the constructor of TelcoinDistributor contract

Medium

Summary

The TelcoinDistributor smart contract, as presented, contains a vulnerability in the constructor function that allows for the assignment of a negative value to the challengePeriod variable, which should represent a time duration.

Vulnerability Detail

In the TelcoinDistributor contract's constructor function, the challengePeriod variable is initialized without proper validation. This allows for the assignment of negative values to challengePeriod, which is intended to represent a time duration in seconds.

Impact

Allowing the challengePeriod to be assigned a negative value can have a significant impact on the behavior of the TelcoinDistributor contract. Negative time durations can lead to unpredictable behavior and may result in incorrect challenge period calculations. This potentially will disrupt the intended operation of the contract and compromise its security.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L64

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, it is recommended to add a validation check in the constructor to ensure that the challengePeriod is assigned a positive value. This can be done by adding the following require statement:

    constructor(
        IERC20 telcoin,
        uint256 period,
        IERC721 council
    ) Ownable(_msgSender()) {
        // verifies no zero values were used
        require(
            address(telcoin) != address(0) &&
                address(council) != address(0) &&
-               period != 0,                     // A negative value can be assigned
+               period > 0,
           "TelcoinDistributor: cannot intialize to zero"
        );
        // initialize telcoin address
        TELCOIN = telcoin;
        // Initialize challengePeriod duration
        challengePeriod = period;
        // Initialize councilNft address
        councilNft = council;

        // Emitting an event after proposing a transaction
        emit ChallengePeriodUpdated(challengePeriod);
    }

By implementing this change, the contract will only accept positive values for the challengePeriod variable, enhancing the security and reliability of the TelcoinDistributor contract.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { This is invalid because constructor is not user control function and thus wont be able to call it; admin will make sure its never negative when setting}

[nevillehuang]

Invalid, period is a uint256 that can only take positive values.