Use of Deprecated `safeApprove` Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xae0fd30af2ae311eb36b763ae8236a0dff7bc3ebad874e1dbc5040ff4b150dbd Severity: low

Description: Description

The contract is using the deprecated safeApprove function from the SafeTransferLib library. This function has been replaced by safeIncreaseAllowance and safeDecreaseAllowance to mitigate potential issues with certain ERC20 implementations. Using deprecated functions can lead to unexpected behavior and potential vulnerabilities in the future if bugs are discovered in the deprecated function.

While there is no direct attack vector from using safeApprove, it could potentially lead to issues with certain ERC20 tokens that have implemented additional checks in their approve function. For example, some tokens require the current allowance to be zero before setting a new non-zero allowance.

In such cases, safeApprove might fail unexpectedly, leading to transaction reverts and poor user experience.

Proof of Concept

function safeApprove(
    IERC20 token,
    address to,
    uint256 amount
) internal {
    bool success;

    assembly {
        // Get a pointer to some free memory.
        let freeMemoryPointer := mload(0x40)

        // Write the abi-encoded calldata into memory, beginning with the function selector.
        mstore(freeMemoryPointer, 0x095ea7b300000000000000000000000000000000000000000000000000000000)
        mstore(add(freeMemoryPointer, 4), to) // Append the "to" argument.
        mstore(add(freeMemoryPointer, 36), amount) // Append the "amount" argument.

        success := and(
            // Set success to whether the call reverted, if not we check it either
            // returned exactly 1 (can't just be non-zero data), or had no return data.
            or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
            // We use 68 because the length of our calldata totals up like so: 4 + 32 * 2.
            // We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
            // Counterintuitively, this call must be positioned second to the or() call in the
            // surrounding and() call or else returndatasize() will be zero during the computation.
            call(gas(), token, 0, freeMemoryPointer, 68, 0, 32)
        )
    }

    require(success, "APPROVE_FAILED");
}

Revised Code Suggestion

- function safeApprove(
-     IERC20 token,
-     address to,
-     uint256 amount
- ) internal {
-     bool success;
- 
-     assembly {
-         // Get a pointer to some free memory.
-         let freeMemoryPointer := mload(0x40)
- 
-         // Write the abi-encoded calldata into memory, beginning with the function selector.
-         mstore(freeMemoryPointer, 0x095ea7b300000000000000000000000000000000000000000000000000000000)
-         mstore(add(freeMemoryPointer, 4), to) // Append the "to" argument.
-         mstore(add(freeMemoryPointer, 36), amount) // Append the "amount" argument.
- 
-         success := and(
-             // Set success to whether the call reverted, if not we check it either
-             // returned exactly 1 (can't just be non-zero data), or had no return data.
-             or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
-             // We use 68 because the length of our calldata totals up like so: 4 + 32 * 2.
-             // We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
-             // Counterintuitively, this call must be positioned second to the or() call in the
-             // surrounding and() call or else returndatasize() will be zero during the computation.
-             call(gas(), token, 0, freeMemoryPointer, 68, 0, 32)
-         )
-     }
- 
-     require(success, "APPROVE_FAILED");
- }

+ // Replace safeApprove with safeIncreaseAllowance and safeDecreaseAllowance
+ function safeIncreaseAllowance(
+     IERC20 token,
+     address spender,
+     uint256 value
+ ) internal {
+     uint256 newAllowance = token.allowance(address(this), spender) + value;
+     _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
+ }
+
+ function safeDecreaseAllowance(
+     IERC20 token,
+     address spender,
+     uint256 value
+ ) internal {
+     unchecked {
+         uint256 oldAllowance = token.allowance(address(this), spender);
+         require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
+         uint256 newAllowance = oldAllowance - value;
+         _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
+     }
+ }
+
+ function _callOptionalReturn(IERC20 token, bytes memory data) private {
+     bytes memory returndata = address(token).functionCall(data, "SafeERC20: low-level call failed");
+     if (returndata.length > 0) {
+         require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
+     }
+ }

// Comment: The deprecated safeApprove function has been replaced with safeIncreaseAllowance and safeDecreaseAllowance.
// These new functions handle allowance changes more safely, especially for tokens that require the current allowance
// to be zero before setting a new non-zero allowance. The _callOptionalReturn function is added to handle the low-level
// calls to the token contract safely.

This revised code replaces the deprecated safeApprove function with safeIncreaseAllowance and safeDecreaseAllowance, which are safer alternatives for managing token allowances. The new implementation also includes a helper function _callOptionalReturn to handle the low-level calls to the token contract safely.

[0xRizwan]

Non-issue Ref