search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

eeshenggoh - Missing fallback() in Safe Guard incase of Safe upgrade causing the Safe to be locked #128

Closed sherlock-admin closed 5 months ago

sherlock-admin commented 5 months ago

eeshenggoh

high

Missing fallback() in Safe Guard incase of Safe upgrade causing the Safe to be locked

Summary

Safe Guards can perform checks both before and after a Safe transaction. However, it overlooked the possibility of a Safe upgrade, which could result in the Safe being locked.

Vulnerability Detail

The absence of a fallback() function in SafeGuard.sol poses a potential vulnerability. This function is crucial in scenarios where the expected check method might change, leading to the risk of locking the Safe. Without a fallback function, the contract cannot receive Ether or more importantly handle calls to undefined functions, a limitation that could hinder the upgrade process.

The decision not to include a fallback function aims to prevent complications during Safe upgrades, where a reverting fallback could inadvertently lock the Safe, especially if there are changes in the expected check method.

Impact

Any transaction called to the Safe will be denied.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/zodiac/core/SafeGuard.sol#L13

Tool used

Manual Review

Recommendation

Include a fallback function in SafeGuard.sol. Reference

sherlock-admin2 commented 5 months ago

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

takarez commented:

invalid because { This is invalid as the contract is not mentioned to recieve ether}

nevillehuang commented 5 months ago

request poc

sherlock-admin2 commented 5 months ago

PoC requested from @goheesheng

Requests remaining: 8

amshirif commented 5 months ago

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

goheesheng commented 5 months ago

For PoC: Anyone who interacts with the SafeGuard, will have transactions checked. Let's say that there is an upgrade, and the check method changed. Anyone who calls the contract will interact with Guards, hence, the transaction will always fail. Safe will lock the funds, because of changed implementation. Hence funds will be stuck and deemed inaccessible.

As stated by the dev team: "It's there as a safety measure. The intent is not that the fallback is used, rather that it doesn't cause issues in the case that safe changes the way it interacts with guards; we don't want to accidentally cause bricked safes."

Would like to escalate to a High as funds will be locked in the safe permanently as I had discussed with the @ZodiacContributor as well as @GnosisGuild via Discord. Do talk to them for further verification if required.Official Discord Link

nevillehuang commented 5 months ago

@goheesheng @amshirif I am invalidating this issue, this is a future integration issue not explicitly mentioned in the docs by telcoin. By current means of code logic, there is no risk to the protocol.

goheesheng commented 5 months ago

@goheesheng @amshirif I am invalidating this issue, this is a future integration issue not explicitly mentioned in the docs by telcoin. By current means of code logic, there is no risk to the protocol.

Hi, I think I wasn't explicitly clear, the problem arises when the Sablier protocol safeguard module has an update, which affects the current SafeGuard module because of the transaction called towards Sablier.

nevillehuang commented 5 months ago

@goheesheng That will fall under external admin actions breaking certain assumptions. This admins are trusted, so this would fall under this sherlock rules. I will be maintaining as invalid, please feel free to escalate during escalation period.

  1. An admin action can break certain assumptions about the functioning of the code.
goheesheng commented 5 months ago

@goheesheng That will fall under external admin actions breaking certain assumptions. This admins are trusted, so this would fall under this sherlock rules

  1. An admin action can break certain assumptions about the functioning of the code.

It doesn't fall under that category. Sablier updates is not controlled by Telecoin protocol so as any admins of Telecoin's. This causes the function in Telecoin to not work at all because of the wrongful integration/implementation of fallback not included, which of course has been fixed by @amshirif

nevillehuang commented 5 months ago

@Czar102 Can you take a look at this too? I am inclined to keep invalid.

amshirif commented 5 months ago

I went ahead and included the suggestion because it was simple but I also feel that it's invalid.

Czar102 commented 5 months ago

@nevillehuang from my understanding this is invalid since it's falling under an external admin rules exactly as you said.

@goheesheng Is there any reasonable justification (maybe Sablier docs) that they may upgrade this function in the future?

goheesheng commented 5 months ago

@nevillehuang from my understanding this is invalid since it's falling under an external admin rules exactly as you said.

@goheesheng Is there any reasonable justification (maybe Sablier docs) that they may upgrade this function in the future? Spoken with Sabliers Team they stated that they it is depreciated an will not be fixing it as well. The following text: "I'll say this at start: I see that your are questionsregarding Proxy architecture which we no longer use. I would highly suggest you to do the same. (it is not even documentated on our docs anymore). But if you really want to learn about it, here is an old version of our docs site: https://v2-docs-l5scktj5j-sablier.vercel.app/contracts/v2/guides/proxy-architecture/overview"