search

code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

Deprecated safeApprove() function #193

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashBase.sol#L68 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashBase.sol#L73

Vulnerability details

Impact

Using this deprecated function can lead to unintended reverts and potentially the locking of funds and also frontrunings.

Proof of Concept

A deeper discussion on the deprecation of this function is in OZ issue #2219. The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

Tools Used

Manual review

Recommended Mitigation Steps

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead: In wfCashBase.sol, 

68: assetToken.safeApprove(address(NotionalV2), type(uint256).max);
73: underlyingToken.safeApprove(address(NotionalV2), type(uint256).max);
jeffywu commented 2 years ago

This should be a QA report. Since these are called in the initializer inside a smart contract, there cannot be front running. They never enter the mempool.

gzeoneth commented 2 years ago

Consider with #192