bin2chen - recover() using the standard transfer may not be able to retrieve some tokens

[sherlock-admin2]

bin2chen

medium

recover() using the standard transfer may not be able to retrieve some tokens

Summary

in SecondaryRewarder.recover()
Using the standard IERC20.transfer()
If REWARD_TOKEN is like USDT, it will not be able to transfer out, because this kind of token does not return bool
This will cause it to always revert

Vulnerability Detail

SecondaryRewarder.recover() use for

Allows the Notional owner to recover any tokens sent to the address or any reward tokens remaining on the contract in excess of the total rewards emitted.

    function recover(address token, uint256 amount) external onlyOwner {
        if (Constants.ETH_ADDRESS == token) {
            (bool status,) = msg.sender.call{value: amount}("");
            require(status);
        } else {
@>          IERC20(token).transfer(msg.sender, amount);
        }
    }

Using the standard IERC20.transfer() method to execute the transfer
A token of a type similar to USDT has no return value
This will cause the execution of the transfer to always fail

Impact

If REWARD_TOKEN is like USDT, 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

    function recover(address token, uint256 amount) external onlyOwner {
        if (Constants.ETH_ADDRESS == token) {
            (bool status,) = msg.sender.call{value: amount}("");
            require(status);
        } else {
-          IERC20(token).transfer(msg.sender, amount);
+          GenericToken.safeTransferOut(token,msg.sender,amount);
        }
    }

[nevillehuang]

@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?

[sherlock-admin2]

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 to USDT 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.

[AuditorPraise]

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 to USDT 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.

[0xMR0]

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.

[sherlock-admin2]

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.

[Hash01011122]

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.

[AuditorPraise]

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.

[nevillehuang]

@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.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xMR0: accepted

[sherlock-admin]

The protocol team fixed this issue in PR/commit notional-finance/contracts-v3#28.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.