R-Nemes - Use of ERC20 approve without resetting first

[sherlock-admin]

R-Nemes

medium

Use of ERC20 approve without resetting first

Summary

It is recommended to reduce the spender allowance to zero before each approve(, ) call.

Vulnerability Detail

batchAuctionParams_.payoutTokenParams.underlying.approve(address(teller()), amount); function is called without setting the allowance to zero. Some tokens, like USDT, require first reducing the address' allowance to zero by calling approve(_spender, 0).

Impact

Some tokens, like USDT, require first reducing the address' allowance to zero by calling approve(_spender, 0). Transactions will revert when using an unsupported token like USDT

Code Snippet

bonds/src/BondBatchAuctionV1.sol#L151

Tool used

Manual review

Recommendation

Reset the approval to zero first

diff --git a/bonds/src/BondBatchAuctionV1.sol b/bonds/src/BondBatchAuctionV1.sol
index faf3d49..02383e8 100644
--- a/bonds/src/BondBatchAuctionV1.sol
+++ b/bonds/src/BondBatchAuctionV1.sol
@@ -148,6 +148,7 @@ contract BondBatchAuctionV1 is IBondBatchAuctionV1, ReentrancyGuard, Clone {
             ) revert BatchAuction_TokenNotSupported();

             // Approve the teller for the amount with fee and create Bond Tokens
+            batchAuctionParams_.payoutTokenParams.underlying.approve(address(teller()), 0);
             batchAuctionParams_.payoutTokenParams.underlying.approve(address(teller()), amount);
             teller().create(
                 batchAuctionParams_.payoutTokenParams.underlying,

Duplicate of #5

[Oighty]

Disagree with this finding.

The approval and transfer sequence of the underlying token within initiateBatchAuction from the BatchAuctionV1 contract to the BondFixedExpiryTeller contract is a closed loop which always results in the Teller's allowance of the BatchAuction's underlying tokens to be zero at the end of the function. No other functions can interact with the Teller and increase/decrease the allowance. Therefore, the approval will never revert because of an existing non-zero allowance unless the token fails to conform to the ERC20 standard (which isn't be supported).

• At the start, allowance = 0.
• allowance is increased from 0 to amount on line 151 by calling underlying.approve(address(teller()), amount);
• allowance is decreased from amount to 0 on line 152 by calling teller().create(underlying, expiry, amount since teller.create() pulls the underlying tokens with safeTransferFrom(msg.sender, address(this), amount)
• No other actions are performed on underlying, therefore allowance = 0 at the end.