Is not following the SafeERC20 rule.

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0xf06a90d82ba79b0685abde5ac113a18355390a4d170c2bb137f29688d81ea7b3
Severity: high

Description:

Description

The Router.sol contract does not implementing the SafeERC20::forceApprove() correctly. As per the SafeERC20::forceApprove() natspec the dev must need to set the allowance to 0 before using forceApprove(), see it here:

Meant to be used with tokens that require the approval
to be set to zero before setting it to a non-zero value, such as USDT

So if USDT is used as token here so the approval should be set to 0 before applying the forceApprove.

Affected code

if (allowance < repayAmount) {
            // Approve the lender to pull the funds if needed
            IERC20(_token).forceApprove(msg.sender, repayAmount);
}

PoC

Spectra-0x4b792db3d2a5d1c1ccf9938380756b200c240e5d/src/router/Router.sol

Lines 280 to 283 in d245fd6

	if (allowance < repayAmount) {
	// Approve the lender to pull the funds if needed
	IERC20(_token).forceApprove(msg.sender, repayAmount);
	}

[yanisepfl]

Hello,

We classified this issue as Invalid.

Indeed, the SafeERC20::forceApprove() function is explicitly designed to handle tokens like USDT that require the allowance to be reset to zero before setting it to a new value. Its implementation ensures that the intermediate step of setting the allowance to zero is automatically handled. Thus, developers do not need to manually set the allowance to zero when using forceApprove().

When forceApprove() is invoked, it does the following:

• Checks the current allowance.
• If the allowance is not equal to the desired amount, it first sets the allowance to zero (if required by the token logic).
• Then, it sets the allowance to the desired non-zero value.

This behavior satisfies the requirements of tokens like USDT without requiring additional manual intervention from the us.

The NatSpec comment in SafeERC20::forceApprove() was misunderstood by the reporter. Indeed, it explains the purpose of the method but does not imply that we need to explicitly call approve(0) before using forceApprove(). The method encapsulates this behavior internally.

Thanks