0xadrii - Pausing the TelcoinDistributor can prevent the challenge period mechanism from functioning properly

[sherlock-admin2]

0xadrii

medium

Pausing the TelcoinDistributor can prevent the challenge period mechanism from functioning properly

Summary

The contract's paused time is not considered when challenging a proposal, which could lead to proposals being executed without having a period where council members can challenge it.

Vulnerability Detail

The TelcoinDistributor.sol allows new transactions to be proposed. Council members can challenge proposals using the challengeTransaction() function:

// TelcoinDistributor.sol

function challengeTransaction(
        uint256 transactionId
    ) external onlyCouncilMember whenNotPaused {
        // Makes sure the id exists
        require(
            transactionId < proposedTransactions.length,
            "TelcoinDistributor: Invalid index"
        );

        // 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"
        );

        // Sets the challenged flag of the proposed transaction to true
        proposedTransactions[transactionId].challenged = true;

        // Emits an event with the transaction ID and the challenger's address
        emit TransactionChallenged(transactionId, _msgSender());
    }

This mechanism is crucial in the TelcoinDistributor mechanics, given that any council member can propose transactions at their will. If a transaction is challenged, it will no longer be able to be executed using the executeTransaction(), so this is an important component to prevent malicious transactions from being executed.

The problem with the current implementation is that pausing the contract can make the challenge time for a transaction to be passed, preventing users from actually challenging the transaction. Note that the challengeTransaction() has the whenNotPaused modifier, which prevents it from being called if the contract is paused.

Consider the following scenario:

1. A transaction is proposed. At this point, any council member can challenge it until the challengePeriod passes.
2. After the transaction gets proposed, the owner of the contract is forced to pause the contract due to an external unfortunate event.
3. After some time (a time greater than the challengePeriod) passes, the owner decides to unpause the contract.
4. Because the challenging period for the transaction proposed in 1 has passed while the contract was in a paused state, council members will no longer be able of challenging that transaction. If the transaction is malicious, it could have unfortunate consequences for the protocol (such as draining the owner() ’s TELCOIN balance).

The following diagram illustrates the previously described scenario:

Impact

Medium. Although this is an edge case that might have critical consequences, the probability of such a situation taking place is low. However, this situation can not be overlooked and must be considered, given that it impacts a crucial protocol mechanic.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider tracking the contract’s paused time. Such time should be considered in the challengePeriod function in order to decide if the challenge period should be extended or not for the currently proposed transactions. If the contract was paused between a transaction proposal and its total challenge period duration, the paused difference should be considered in order to apply a proper challenge period duration.

Duplicate of #24

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid and due to the same reasol wiith issue 024}