Tokens that don't actually perform the approve and return false are still counted as a correct approve.
Proof of Concept
In NotionalTradeModule.sol, there is _approve() function which makes a low level call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.
GitHub Reference
It is usually good to add a require-statement that checks the return value or to use something like safeApprove; unless one is sure the given token reverts in case of a failure. Also recommended using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.
ckoopmann
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L502
Vulnerability details
Impact
Tokens that don't actually perform the approve and return false are still counted as a correct approve.
Proof of Concept
In NotionalTradeModule.sol, there is
_approve()function which makes a low level call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead. GitHub ReferenceReference to previous C4 finding
Tools Used
Manual review
Recommended Mitigation Steps
It is usually good to add a require-statement that checks the return value or to use something like safeApprove; unless one is sure the given token reverts in case of a failure. Also recommended using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.