HonorLt - Challenge period immediate effect

[sherlock-admin2]

HonorLt

medium

Challenge period immediate effect

Summary

Setting a new challenge period takes immediate effect even for old proposals.

Vulnerability Detail

An owner can set the challenge period at any time to any value:

    function setChallengePeriod(uint256 newPeriod) public onlyOwner {
        //update period
        challengePeriod = newPeriod;
        // Emitting an event for new period
        emit ChallengePeriodUpdated(challengePeriod);
    }

This change directly impacts not only new proposals but also old ones, e.g. when challenging txs:

        // Reverts if the current time exceeds the sum of the transaction's timestamp and the challenge period
        require(
            block.timestamp <=
                proposedTransactions[transactionId].timestamp + challengePeriod,
            "TelcoinDistributor: Challenge period has ended"
        );

or when executing:

        // Reverts if the challenge period has not expired
        require(
            block.timestamp >
                proposedTransactions[transactionId].timestamp + challengePeriod,
            "TelcoinDistributor: Challenge period has not ended"
        );

challengePeriod is always dynamically added to the proposal timestamp. Thus any update to this value immediately affects all proposals. In my opinion, old proposals should stay with the old config, and only new proposals take this updated value.

Impact

Challengers should actively monitor for changes. They might have less time than they initially thought to challenge malicious proposals. Also, a tx might be executed earlier or later than anticipated. What is more, there might happen a situation, if the period is increased, an already executed tx can be challenged, leading to the misleading state.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

Cache current challenge expiration when creating a proposal. Additionally, consider introducing reasonable min/max boundaries for challengePeriod.

[sherlock-admin2]

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

takarez commented:

invalid because { Watson's oppinion should not be ahead of an intended protocol's behavior; the intention is to change the period and thus that applies to all includding existing ones}

[nevillehuang]

Invalid, this is expected behavior. Additionally, challenge period is an admin only function