p-tsanev - TelcoinDistributor.sol - Pausing the contract would disable challenging transactions

[sherlock-admin]

p-tsanev

medium

TelcoinDistributor.sol - Pausing the contract would disable challenging transactions

Summary

The TelcoinDistributor contract allows for the proposal, execution and challenging(cancellation) of transactions. It also introduces pausing functionality, probably to deal with external integration pausing and as a security measure. This functionality can give unfair advantage to proposers.

Vulnerability Detail

The functions for executing and creating proposals are correctly safe-guarded with the whenNotPaused modifier, stopping the creation and execution of proposals. But an unfair advantage is created because the challengeTransaction function has the whenNotPaused modifier as well.
Depending the the duration of the pause it is highly possible for proposals created before the pause, either intentionally via front-running or accidentally, to pass their challenge period, giving council members no way to challenge. Thus creating an unfair advantage during the pause and potentially allowing malicious/unfavorable transactions to reach execution.

Impact

Unfair advantage during pause, potential stealing of funds from the owner() due to inability to challenge proposal

Code Snippet

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

Tool used

Manual Review

Recommendation

Remove the whenNotPaused modifier from the challengeTransaction function.

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/29

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid due to the fact that sherlokc's rule number 5 stated that "Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue."}

[0xMR0]

Escalate

This is invalid issue. The contract can only be paused by owner of contract and the owner is trusted.

Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

TRUSTED

Even if the external integration is considered, Pausing of contract is acceptable to protocol team,

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Acceptable

Let me highlight Sherlock rules for trusted admin resulting in invalid issues,

Protocol admin is considered to be trusted in most cases, hence issues where . . . .

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Since, admin is trusted and it is expected from admin that before pausing the contract, all council members would be notified via some medium and it can be further assumed that all pending proposal would be executed before pausing the contract and there is no such issue can happen except malicious owner purposefully allows it council members.

I think, the issue could be valid if the admin was restricted with specific mention of pausing the contract in contest readme.md.

Feel free to correct, if i am missing something.

[sherlock-admin]

Escalate

This is invalid issue. The contract can only be paused by owner of contract and the owner is trusted.

Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

TRUSTED

Even if the external integration is considered, Pausing of contract is acceptable to protocol team,

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Acceptable

Let me highlight Sherlock rules for trusted admin resulting in invalid issues,

Protocol admin is considered to be trusted in most cases, hence issues where . . . .

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Since, admin is trusted and it is expected from admin that before pausing the contract, all council members would be notified via some medium and it can be further assumed that all pending proposal would be executed before pausing the contract and there is no such issue can happen except malicious owner purposefully allows it council members.

I think, the issue could be valid if the admin was restricted with specific mention of pausing the contract in contest readme.md.

Feel free to correct, if i am missing something.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xLogos]

I would like to say that it can be mitigated by admin by waiting malicious transaction to expire in paused state. I think it should remain medium if 1. waiting for expiration is not acceptable (maybe too long) or 2. expiration of other legitimate transactions is not acceptable

@0xMR0 council member can be malicious

all council members would be notified via some medium and it can be further assumed that all pending proposal would be executed before pausing the contract

[PlamenTSV]

Escalate
Admin does not need to be untrusted. Even if he notifies the council members for the pause, it in no way stops a malicious council member from submitting a malicious proposal that CANNOT be challenged (via front-running). The admin assumption when pausing would be that even if he did, malicious proposals could be challenged but they CANNOT be. An admin cannot know which proposals are malicious before pausing.
Yet another issue that is fixed by the team, I am fairly certain you are wasting your escalation.

[sherlock-admin]

Escalate
Admin does not need to be untrusted. Even if he notifies the council members for the pause, it in no way stops a malicious council member from submitting a malicious proposal that CANNOT be challenged (via front-running). The admin assumption when pausing would be that even if he did, malicious proposals could be challenged but they CANNOT be. An admin cannot know which proposals are malicious before pausing.
Yet another issue that is fixed by the team, I am fairly certain you are wasting your escalation.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

This should remain medium severity based off @0xLogos comments. He brought up really good points, but you can see that point 1 directly contradicts with point 2. Waiting for too long will cause legitimate proposals to be unfairly excluded too, and then the same situation can happen again when a pause is again executed (be it accidental or on purpose proposal proposed before pause is invoked), so to me this breaks core contract functionality, because you cannot make sure that all of the legitimate proposals are executed while waiting for expiration.

Although pausing is a sensitive and rare situation, because this directly undermines the execution of legitimate proposals and possibly even allow malicious proposals to go through, I believe this warrants medium severity.

[amshirif]

I agree with @nevillehuang

[0xMR0]

Respectfully disagree with above comments.

1. The scenario mentioned in above comments is only possible when contract pause period is greater than challenge period which is highly unlikely to happen. If i remember correctly, the sponsor had confirmed the challenge period would be between 3 days to 7 days in public discord channel. So considering both point of 0xlogos, it means the contract pause period would be more than 3/7 days which i believe not going to happen as the protocol wont pause the contract for more than 1 day since pausing the contract is rare and highly likely only in emergency condition. Another reason, the protocol wont pause contract longer otherwise legitimate proposals wont be created and executed.

2. Council members are semi trusted, either they can pass malicious proposals or legitimate proposals or they can even challenge all proposals so its upto the community to decide what they want. Therefore, saying malicious proposals would be passed due to pause of contract is void here.

3. Admin/owner actions are trusted for this audit which means admin understand good enough the implication of pausing the contract more than the challenge period, Therefore, i believe the contract pause will always be less than the challenge period. The admin/owner can pause or unpause the contract anytime. This trusted admin rule at sherlock also make the issue invalid.

While i agree, the issue could be low severity but its invalid at sherlock. I would agree with @Czar102 final judgment on this issue without further arguments.

[PlamenTSV]

Respectfully disagree with the above comment:

1. You are basing you argument on the assumption how long the pause will be, thus your argument is pretty much void. Sponsor comments do not hold as a source of truth and for as far as we know, the pause could be a long one depending on the emergency that caused it.
2. Council members are semi-trusted and can be forcefully removed when malicious activity is detected. The idea is they are unable to execute it, which is not the case here since there is a viable and real possible scenario in which they can do so, THAT'S CONFIRMED BY THE SPONSOR AND FIXED (a hint not to discuss this further)
3. Trusted admin has nothing to do with the issue, you can not determine how, when and why the contract would be paused in case of an emergency and for how long that would be until the problem the pause was initiated for was resolved. I trust the admin he would not pause the contract for his own benefit, I do not trust the other council members they would not exploit this pause. And now they can't.
   All of your arguments are null at this points, the sponsors and lead judge confirm the issue, while still abiding for the same rules you non-stop link. We will wait on the final judgement, end of discussion.

[nevillehuang]

Agree with @PlamenTSV the points made by @0xMR0 are simply assumptions and too dangerous, especially point 2, semi trusted does not mean they can decide whatever they want. This is why a challenge mechanism exists in the first place and should be applied consistently, be it in a paused state or not.

[Evert0x]

I don't believe this issue should be rewarded.

I believe it's a design choice to pause the challengeTransaction function. The admin is trusted to use this functionality in the best interest of the protocol.

This issue illustrates a scenario where pausing the challengeTransaction function is a bad thing. But it could also be a good thing in case there is a security accident which can only be mitigated by pausing this function.

I believe this language applies

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

[Evert0x]

Result:
Invalid
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xMR0: accepted
• GerhardBotha97: rejected

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/29.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.