Open sherlock-admin2 opened 9 months ago
@jeffywu @T-Woodward Were there any publicly available information stating USDT won't be use as a potential reward tokens at the point of the contest?
Escalate Upon closer examination and in alignment with the Sherlock rules, it becomes evident that issues of this nature are categorically classified under informational issues. Furthermore, should we acknowledge the concerns surrounding
safeTransferOut
due toUSDT
peculiarities, it is imperative to underscore that, at most, this warrants a classification of low severity. This is primarily because the core functionality of the protocol remains unaffected and fully operational without getting bricked.
You've deleted an escalation for this issue.
Escalate Upon closer examination and in alignment with the Sherlock rules, it becomes evident that issues of this nature are categorically classified under informational issues. Furthermore, should we acknowledge the concerns surrounding
safeTransferOut
due toUSDT
peculiarities, it is imperative to underscore that, at most, this warrants a classification of low severity. This is primarily because the core functionality of the protocol remains unaffected and fully operational without getting bricked.
"Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README"
contest readme::
Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?
USDC and USDT are the primary examples.
Escalate
This is indeed a valid issue since the non-standard behavior of USDT is not acceptable to protocol team and it is explicitly mentioned in contest readme.
Further, comment by @AuditorPraise is correct and enough for the validation of this issue.
Escalate
This is indeed a valid issue since the non-standard behavior of USDT is not acceptable to protocol team and it is explicitly mentioned in contest readme.
Further, comment by @AuditorPraise is correct and enough for the validation of this issue.
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.
IMO In my opinion, the issue with safeTransfer
has been widely recognized since 2022. Furthermore, it seems unfair to Watson who are discovering different vulnerabilities along with their attack paths. For instance, the issue of hardcoded chainId
was previously classified as high severity. However, as seen in this issue, it was downgraded to low severity due to its widespread awareness and ease of discovery.
As mentioned in sherlock rules: Low/Informational Issues: While Sherlock acknowledges that it would be great to include & reward low-impact/informational issues, we strongly feel that Watsons should focus on finding the most critical vulnerabilities that will potentially cause millions of dollars of losses on mainnet. Sherlock understands that it could be missing out on some potential "value add" for protocol, but it's only because the real task of finding critical vulnerabilities requires 100% of the attention of Watsons. While low/informational issues are not rewarded individually if a Watson identifies an attack vector that combines multiple lows to cause significant loss/damage that would still be categorized as a valid medium/high.
IMO In my opinion, the issue with
safeTransfer
has been widely recognized since 2022.
The issue isn't about safeTransfer
but USDT.
It's no one's fault that devs still make such mistakes... But that doesn't reduce the risks associated with making such a mistake. The impact it has had on protocols since 2022 till now remains the same.
Funds could be stuck
So why should it be an informational now?
You can't compare chain Id issue to USDT being stuck in a contract as a result of the devs not using safeTransfer.
@Hash01011122 You are circling too much around sherlock rules, and should look at it more factually instead of subjectively. In the contest details/code logic/documentation, no place does it mention that USDT cannot be a reward token, so I believe your argument is basically invalid. I believe no further discussions is required from my side, imo, this should remain medium severity.
Result: Medium Has Duplicates
The protocol team fixed this issue in PR/commit https://github.com/notional-finance/contracts-v3/pull/28.
The Lead Senior Watson signed-off on the fix.
bin2chen
medium
recover() using the standard transfer may not be able to retrieve some tokens
Summary
in
SecondaryRewarder.recover()
Using the standardIERC20.transfer()
IfREWARD_TOKEN
is likeUSDT
, it will not be able to transfer out, because this kind oftoken
does not returnbool
This will cause it to alwaysrevert
Vulnerability Detail
SecondaryRewarder.recover()
use forUsing the standard
IERC20.transfer()
method to execute the transfer Atoken
of a type similar toUSDT
has no return value This will cause the execution of the transfer to always failImpact
If
REWARD_TOKEN
is likeUSDT
, it will not be able to transfer out.Code Snippet
https://github.com/sherlock-audit/2023-12-notional-update-5/blob/main/contracts-v3/contracts/external/adapters/SecondaryRewarder.sol#L152C3-L159
Tool used
Manual Review
Recommendation