code-423n4 / 2021-11-fairside-findings

0 stars 0 forks source link

safeApprove is deprecated. #16

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pants

Vulnerability details

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) IERC20(DAI).approve(address(uniswap), amount);

FSD.sol line 406

YunChe404 commented 2 years ago

safeApprove is deprecated as it does not work as the rest of the safe functions. Keyword here is deprecated...

pauliax commented 2 years ago

safeApprove is still widely used across contracts. Warden didn't mention any concrete attack paths and provided an invalid mitigation solution, using .approve is also discouraged, OZ suggests using safeIncreaseAllowance and safeDecreaseAllowance where possible. Marking this issue as non-critical.

YunChe404 commented 2 years ago

As the approval occurs right before the Uniswap exchange, the approval will be consumed in full meaning that the value will always be set from zero to non-zero rendering the usage of safeIncreaseAllowance counter-intuitive and gas-inefficient.

pauliax commented 2 years ago

Agree, leaving this issue as a non-critical recommendation.