Must approve 0 before setting approve

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/yearn/YearnAdapter.sol#L54

Vulnerability details

Impact

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved. Otherwise it will fail.

Proof of Concept

The yearn adapter its an upgreadable proxy, this means that the address approving the token will always be the same, so the current initialize will work only once.

    /**
     * @notice Initialize a new Yearn Adapter.
     * @param adapterInitData Encoded data for the base adapter initialization.
     * @param externalRegistry Yearn registry address.
     * @dev This function is called by the factory contract when deploying a new vault.
     * @dev The yearn registry will be used given the `asset` from `adapterInitData` to find the latest yVault.
     */
    function initialize(
        bytes memory adapterInitData,
        address externalRegistry,
        bytes memory
    ) external initializer {
        // remove code for readability

        IERC20(_asset).approve(address(yVault), type(uint256).max);
    }

If the proxy has a previous implementation that has approve the token, and then you change the implementation it will fail on the approve

Tools Used

manual revision

Recommended Mitigation Steps

Reset approval before approving;

diff --git a/src/vault/adapter/yearn/YearnAdapter.sol b/src/vault/adapter/yearn/YearnAdapter.sol
index d951e63..64e9957 100644
--- a/src/vault/adapter/yearn/YearnAdapter.sol
+++ b/src/vault/adapter/yearn/YearnAdapter.sol
@@ -51,6 +51,7 @@ contract YearnAdapter is AdapterBase {
         );
         _symbol = string.concat("popY-", IERC20Metadata(asset()).symbol());

+        IERC20(_asset).approve(address(yVault), 0);
         IERC20(_asset).approve(address(yVault), type(uint256).max);
     }

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as unsatisfactory: Out of scope