Tricko - `whenNotPaused` modifier in `TelcoinDistributor.challengeTransaction()` facilitates the execution of malicious proposals.

[sherlock-admin2]

Tricko

medium

whenNotPaused modifier in TelcoinDistributor.challengeTransaction() facilitates the execution of malicious proposals.

Summary

Due to the whenNotPaused modifier present in TelcoinDistributor.challengeTransaction(), when the TelcoinDistributor contract is paused the effective challenge period that is shorter than intended, facilitating the execution of malicious proposals.

Vulnerability Detail

The lifecycle of a TelcoinDistributor distribution involves several stages. Firstly, any council member can propose a distribution by utilizing the TelcoinDistributor.proposeTransaction() function. Following this proposal, the distribution remains dormant during the challenge period, which spans from 1 to 7 days (as informed by the sponsor). Finally, the proposed distribution is executed through the invocation of TelcoinDistributor.executeTransaction().

Throughout the challenge period, the proposal is in a state where execution is prohibited, opening up the opportunity for other council members to challenge it using the TelcoinDistributor.challengeTransaction() function. Notably, the current implementation includes the whenNotPaused modifier in the challengeTransaction() function, indicating that it is only executable when the contract is in an unpause state.

However, it is crucial for challengeTransaction() to remain unaffected by pauses. This ensures that a malicious proposal cannot be expedited for execution. During a pause, challenges are not accepted, resulting in an effective challenge period that is shorter than intended. In extreme cases, where the pause duration surpasses the challenge period, a malicious council member can propose and execute without any opportunity from other council members to challenge his proposal.

Consider the following scenatio where the attacker (who is also a council member) wants to propose and execute a malicious proposal on a TelcoinDistributor with 48h challenge period. Attacker keeps monitoring the mempool, waiting for the owner to call TelcoinDistributor.pause()

1. (t = 0h) An soon as the attacker sees the owner's pause() transaction, the attacker tries to frontrun it by proposing his malicious transaction by calling proposeTransaction(...).

If his frontrun is sucessfull, his proposal would be added to proposedTransactions array and the contract will be paused. The attacker continues monitoring the mempool but now for the unpause() transaction.

2. (t = 50h) Owner sends unpause() transaction after 50h.
3. (t = 50h) Attacker calls TelcoinDistributor.executeTransaction(...) trying to backrun the owner's transaction, thus executing his malicious proposal as soon as the contract is unpaused, thwarting any attempt to challenge it.

This strategy provides the attacker with an advantage. Throughout the pause duration, no challenges to their transaction proposals are possible, diminishing the effective challenge period and facilitating the execution of malicious proposals.

Impact

On the event of a pause, the effective challenge period is smaller than intended. Creating favorable conditions for the execution of malicious proposals.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L115-L136

Tool used

Manual Review

Recommendation

Consider removing the whenNotPaused modifier from TelcoinDistributor.challengeTransaction(), thus allowing proposals to be challenged even when contract is paused.

Duplicate of #24

[sherlock-admin2]

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

takarez commented:

invalid because { I consider this invalid because of the sherlocks rule mentioned in issue 024 and personally i believe when a contract or a function is paused; the devs menas that most of the funcitonalities will paused in case of a secuirity issue}