Description

The UniswapCoreAssetManager contract's function _transferERC20 both approves and transfers the amount:

function _transferERC20(
    address token,
    address to,
    uint256 amount
) internal {
    if (amount > 0) {
        IERC20(token).forceApprove(to, amount);
        IERC20(token).safeTransfer(to, amount);
    }
}

This means that the receiver will obtain both the funds transferred directly as well as an allowance that they can use to later transfer themselves the same amount again. As long as there are not other approvals overriding the earlier one in the meantime and the asset manager has a sufficient balance, the receiver will thus be able to obtain twice the intended amount.

The function _transferERC20 is only called by _transferAsset in the same contract. That function is in turn called by _transferAssetToVault and _chargeFees. The former of the two only transfers to the vault, which is trusted and whose current code does make use of the approvals; hence, impact is limited. The latter transfers to the relayer and fee manager. The function is called by _transferFundsToVaultWithFeesAndCreateNote, which is in turn called by uniswapCollectFees and uniswapRemoveLiquidity.

Impact

Relayers for calls to uniswapCollectFees and uniswapRemoveLiquidity obtain up to twice the intended amount of fees. However, any amount beyond the intended fee must come from tokens stuck in the respective Uniswap asset-manager contracts, where no tokens should stay at rest under normal conditions.

Recommendations

Remove the approval from _transferERC20:

function _transferERC20(
    address token,
    address to,
    uint256 amount
) internal {
    if (amount > 0) {
-         IERC20(token).forceApprove(to, amount);
        IERC20(token).safeTransfer(to, amount);
    }
}

Remediation

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