popeye - Logical time gap flaw in `TelcoinDistributo::executeTransaction` causes potential oversight of valid challenges made at the end of the challenge period.

[sherlock-admin2]

popeye

high

Logical time gap flaw in TelcoinDistributo::executeTransaction causes potential oversight of valid challenges made at the end of the challenge period.

Summary

Vulnerability Detail

On TelcoinDistributo::executeTransaction , this line checks if the current time exceeds the transaction's timestamp plus the challenge period, indicating that the challenge period has ended.

require(
    block.timestamp > proposedTransactions[transactionId].timestamp + challengePeriod,
    "TelcoinDistributor: Challenge period has not ended"
);

This line checks whether the transaction has been challenged.

require(
    !proposedTransactions[transactionId].challenged,
    "TelcoinDistributor: transaction has been challenged"
);

While the first require statement ensures that the challenge period has ended, it does not account for challenges that may have been submitted right before the end of this period. The contract only verifies if the transaction was challenged before, not considering the exact timing of these challenges.

Suppose a transaction is proposed at timestamp = T. The challenge period lasts until timestamp = T + challengePeriod. If a challenge is submitted very close to T + challengePeriod (say at T + challengePeriod - ε, where ε is a minimal time), it is valid and should prevent the transaction from executing. However, due to the current logic, if executeTransaction is called immediately after T + challengePeriod (even a second later), the contract would allow the transaction to proceed since it only checks if the current time is beyond the challenge period, not whether a challenge was made during this period.

Impact

It can allow potentially contested transactions to be executed, undermining the effectiveness of the challenge mechanism.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L87-L106 https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L115-L136 https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L143-L175

Proof of Concept

Consider two actors, Alice and Bob. Alice proposes a transaction which enters the challenge period. Bob notices an issue with the transaction and decides to challenge it just before the challenge period ends. However, due to the timing gap, Alice executes the transaction immediately after the challenge period ends, but before the challenge from Bob is recognized by the contract. This leads to the execution of a potentially faulty or contentious transaction.

Actors:

• Alice: A council member who proposes a transaction in the TelcoinDistributor.sol contract.
• Bob: Another council member who intends to challenge the transaction proposed by Alice.

Scenario:

1. Alice Proposes a Transaction:

   • At timestamp = t0, Alice calls proposeTransaction to propose a new transaction. This transaction is now open for challenges for a predefined period (challengePeriod).
   • The transaction is stored in the proposedTransactions array with a timestamp of t0.

2. Bob Challenges the Transaction:

   • Just before the challenge period is about to end, at timestamp = t0 + challengePeriod - ε (where ε is a small time interval, say a few seconds), Bob notices an issue and calls challengeTransaction to challenge Alice's transaction.
   • The transaction's challenged flag is set to true. However, the executeTransaction function does not account for the exact timing of this challenge.

3. Alice Executes the Transaction:

   • Immediately after the challenge period supposedly ends, at timestamp = t0 + challengePeriod + δ (where δ is a very small time interval right after the challenge period), Alice calls executeTransaction.
   • The contract checks if the current time is greater than t0 + challengePeriod. Since it is, the contract mistakenly allows the transaction execution, even though Bob's challenge was legitimate and within the challenge period.

Tool used

Manual Review

Recommendation

Modify the executeTransaction function to ensure challenges are accounted for until the very end of the challenge period. Introduce an additional state variable or a mechanism to lock the transaction from execution once challenged.

function executeTransaction(uint256 transactionId) external onlyCouncilMember whenNotPaused {
    require(
        transactionId < proposedTransactions.length,
        "TelcoinDistributor: Invalid index"
    );
    require(
        block.timestamp > proposedTransactions[transactionId].timestamp + challengePeriod,
        "TelcoinDistributor: Challenge period has not ended"
    );
    // Ensure transaction is not challenged at any point during the challenge period
    require(
        !proposedTransactions[transactionId].challenged || 
        block.timestamp > proposedTransactions[transactionId].challengeTimestamp + challengePeriod,
        "TelcoinDistributor: transaction is or was challenged"
    );
    // Rest of the function logic...
}

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid ; the watson failed to explained a clear issue hes intended to report; or there is a alck of explanation of impact also}

[nevillehuang]

Low severity, admins are trusted to set sufficient enough challenge period for council members to challenge, so that they should not challenge transaction at the very last miniute