Zellic - Audit Reports

Category: Business Logic

Incomplete asset-validation logic

Low Severity

Low Impact

Low Likelihood

Description

The _validateAssets function is defined in CurveAssetManagerHelper as follows.

function _validateAssets(
    address[8] memory coins,
    address[4] memory assets
) internal pure returns (bool) {
    for (uint256 i = 0; i < 4; i++) {
        if (coins[i] == address(0)) {
            continue;
        }
        if (assets[i] != address(0)) {
            if (assets[i] != coins[i]) {
                if (
                    assets[i] == _ETH_ADDRESS && coins[i] == _WETH_ADDRESS
                ) {
                    continue;
                }
                return false;
            }
        }
    }
    return true;
}

The following snippet makes the validation logic insufficient.

if (coins[i] == address(0)) {
    continue;
}

Consider a Curve pool with two tokens, poolToken1 and poolToken2. If a user initiates a request where the assets are [poolToken1, poolToken2, extraToken1, extraToken2], this will still pass the _validateAssets check.

Impact

One possible impact is a fund lock in addLiquidity functions. For example, in CurveAddLiquidityAssetManager, _addLiquidity contains the following snippets:

for (i = 0; i < 4; i++) {
    if (actualAmounts[i] > 0) {
        if (_args.assets[i] == ETH_ADDRESS) {
            if (ethAmount > 0) {
                revert AmountNotCorrect();
            }
            _assetPoolETH.release(
                payable(address(this)),
                _args.amounts[i]
            );
            ethAmount = actualAmounts[i];
            // 01 or 11
            if (_args.isLegacy & 1 == 1) {
                _wrapEth(_args.amounts[i]);
                IERC20(_WETH_ADDRESS).forceApprove(
                    _args.pool,
                    actualAmounts[i]
                );
            }
        } else {
            _assetPoolERC20.release(
                _args.assets[i],
                address(this),
                _args.amounts[i]
            );
            IERC20(_args.assets[i]).forceApprove(
                _args.pool,
                actualAmounts[i]
            );
        }
    }
}

for (i = 0; i < 4; i++) {
    _postWithdraw(_args.nullifiers[i]);
}

This pulls assets from the asset pools for all the provided assets. All the corresponding notes are also marked as spent.

But the processing only happens for the first coinNum tokens; the ownership of the remaining two tokens, extraToken1 and extraToken2, is therefore never transferred from the asset-manager contract and gets locked.

Recommendations

Modify the _validateAssets functions to handle extra tokens.

function _validateAssets(
    address[8] memory coins,
    address[4] memory assets
) internal pure returns (bool) {
    for (uint256 i = 0; i < 4; i++) {
        if (assets[i] != address(0)) {
            if (assets[i] != coins[i]) {
                if (
                    assets[i] == _ETH_ADDRESS && coins[i] == _WETH_ADDRESS
                ) {
                    continue;
                }
                return false;
            }
        }
    }
    return true;
}

Remediation

This issue has been acknowledged by Singularity, and a fix was implemented in commit 55c3df63↗.