merlin - WrappedAerodromeAM.sol is not compatible with the Revert on Zero Value Tokens

[sherlock-admin4]

merlin

medium

WrappedAerodromeAM.sol is not compatible with the Revert on Zero Value Tokens

Summary

From the README file: We also want to receive issues regarding minimal implementations of ERC20 Tokens, which besides a standard implementation have one of the following "weird behaviours": Revert on Zero Value.

Vulnerability Detail

When the owner of a position calls WrappedAerodromeAM.decreaseLiquidity, they should receive the transferred liquidity back and fees.

Let's consider the following scenario:

1)The owner of the position calls decreaseLiquidity, and they are supposed to receive fees.

        // Claim any pending fees from the Aerodrome Pool.
        (uint256 fee0Pool, uint256 fee1Pool) = _claimFees(pool);

        // Calculate the new fee balances.
        (poolState_, positionState_) = _getFeeBalances(poolState_, positionState_, fee0Pool, fee1Pool);

2)Since the fee for the revert on zero transfer token is zero, a DOS will occur, and the user will not be able to decrease liquidity.

        // Pay out the fees to the position owner.
        ERC20(token0[pool]).safeTransfer(msg.sender, fee0Position);  //fee0Position = 0
        ERC20(token1[pool]).safeTransfer(msg.sender, fee1Position); // or fee1Position = 0
        emit FeesPaid(positionId, uint128(fee0Position), uint128(fee1Position));

Impact

A denial-of-service (DOS) can occur on the decreaseLiquidity and claimFees functions in WrappedAerodromeAM smart contract of the revert on zero transfer token when the fee is zero value.

Code Snippet

src/asset-modules/Aerodrome-Finance/WrappedAerodromeAM.sol#L452-L453 src/asset-modules/Aerodrome-Finance/WrappedAerodromeAM.sol#L411-L412

Tool used

Manual Review

Recommendation

Consider changing decreaseLiquidity and claimFees functions as follows:

+ if (fee0Position != 0)
            ERC20(token0[pool]).safeTransfer(msg.sender, fee0Position);
+ if (fee1Position != 0)
            ERC20(token1[pool]).safeTransfer(msg.sender, fee1Position);

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

takarez commented:

this should be valid considering the readMe stated to report issues with the said(weird) behavior; medium(1)

shealtielanz commented:

dup of #033

[sherlock-admin3]

The protocol team fixed this issue in the following PRs/commits: https://github.com/arcadia-finance/accounts-v2/pull/202

[oxwhite]

What does "besides a standard implementation have one of the following "weird behaviours": Revert on Zero Value" mean? What ı understand the protocol will not accept any vulnerability related to "Revert on Zero Value" and the followings mentioned in ReadMe, which means it is out of scope. How can this submission be valid?

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.