Brisk Sapphire Chipmunk - Missing ERC1155 Token ID Handling in Withdrawal Logic Leading to inability to withdraw collateral

[sherlock-admin2]

Brisk Sapphire Chipmunk

High

Missing ERC1155 Token ID Handling in Withdrawal Logic Leading to inability to withdraw collateral

Summary

The withdraw function in the CollateralEscrowV1 contract fails to account for the token ID when withdrawing ERC1155 tokens. While the function supports generic collateral withdrawals, it lacks logic to handle ERC1155 tokens that require both a token address and a token ID to uniquely identify a specific asset type.

This poses a serious issue because the protocol supports ERC 1155 to be deposited as collateral as seen in the deposit function in collateralManager.sol;

function _deposit(uint256 _bidId, Collateral memory collateralInfo)
        internal
        virtual
    {
        require(collateralInfo._amount > 0, "Collateral not validated");
        (address escrowAddress, address borrower) = _deployEscrow(_bidId);
        ICollateralEscrowV1 collateralEscrow = ICollateralEscrowV1(
            escrowAddress
        );
        // Pull collateral from borrower & deposit into escrow
        // This will revert with fee-on-transfer tokens 
        if (collateralInfo._collateralType == CollateralType.ERC20) {
            IERC20(collateralInfo._collateralAddress).safeTransferFrom(
                borrower,
                address(this),
                collateralInfo._amount
            );
            IERC20(collateralInfo._collateralAddress).forceApprove(
                escrowAddress,
                collateralInfo._amount
            );
            collateralEscrow.depositAsset(
                CollateralType.ERC20,
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                0
            );
        } else if (collateralInfo._collateralType == CollateralType.ERC721) {
            IERC721Upgradeable(collateralInfo._collateralAddress).transferFrom(
                borrower,
                address(this),
                collateralInfo._tokenId
            );
            IERC721Upgradeable(collateralInfo._collateralAddress).approve(
                escrowAddress,
                collateralInfo._tokenId
            );
            collateralEscrow.depositAsset(
                CollateralType.ERC721,
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                collateralInfo._tokenId
            );
        } else if (collateralInfo._collateralType == CollateralType.ERC1155) {
            bytes memory data;
            IERC1155Upgradeable(collateralInfo._collateralAddress)
                .safeTransferFrom(
                    borrower,
                    address(this),
                    collateralInfo._tokenId,
                    collateralInfo._amount,
                    data
                );
            IERC1155Upgradeable(collateralInfo._collateralAddress)
                .setApprovalForAll(escrowAddress, true);
            collateralEscrow.depositAsset(
                CollateralType.ERC1155,
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                collateralInfo._tokenId
            );
        } else {
            revert("Unexpected collateral type");
        }
        emit CollateralDeposited(
            _bidId,
            collateralInfo._collateralType,
            collateralInfo._collateralAddress,
            collateralInfo._amount,
            collateralInfo._tokenId
        );
    }

Root Cause

In CollaterEscrow.sol: 85, the withdrawal logic assumes that specifying only the collateral address and amount is sufficient for all token types. This assumption breaks for ERC1155 tokens, which require a token ID to differentiate between various token types managed by the same contract.

function withdraw(
    address _collateralAddress,
    uint256 _amount,
    address _recipient
) external virtual onlyOwner {
    require(_amount > 0, "Withdraw amount cannot be zero");
    Collateral storage collateral = collateralBalances[_collateralAddress];
    require(
        collateral._amount >= _amount,
        "No collateral balance for asset"
    );
    _withdrawCollateral(
        collateral,
        _collateralAddress,
        _amount,
        _recipient
    );
    collateral._amount -= _amount;
    emit CollateralWithdrawn(_collateralAddress, _amount, _recipient);
}

The _withdrawCollateral function does not accept or process a tokenId, leaving it incapable of properly handling ERC1155 tokens.

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L85

Internal pre-conditions

1. The collateralBalances mapping uses the _collateralAddress as a key without differentiating between token IDs.
2. The function _withdrawCollateral lacks a parameter or logic for handling ERC1155 token IDs.

External pre-conditions

1. ERC1155 tokens can represent multiple asset types (e.g., token ID 1 for "Gold" and 2 for "Silver") within the same contract.
2. Without the token ID, the contract cannot identify which ERC1155 token type to withdraw, leading to potential errors or unintended behavior.
3. Users may deposit ERC1155 tokens as collateral using the _deposit function but find themselves unable to withdraw them correctly due to the missing token ID logic.

Attack Path

No response

Impact

1. Improper handling of ERC1155 tokens can result in asset loss. This leads to inability to withdraw collateral
2. Protocol Non-Compliance: The withdrawal function deviates from the ERC1155 standard, potentially breaking integrations

PoC

1. A user deposits an ERC1155 token as collateral (e.g., token ID = 1).
2. The user(lender) or protocol attempts to withdraw the collateral using the withdraw function.
3. The function fails or withdraws an unintended token because the token ID is not specified or handled.
4. The user cannot recover their specific collateral.

Mitigation

Update the withdraw Function:
Add a uint256 _tokenId parameter to the withdraw function and propagate it to _withdrawCollateral and any relevant logic.

function withdraw(
    address _collateralAddress,
    uint256 _amount,
    uint256 _tokenId,
    address _recipient
) external virtual onlyOwner {
    require(_amount > 0, "Withdraw amount cannot be zero");
    Collateral storage collateral = collateralBalances[_collateralAddress][_tokenId];
    require(
        collateral._amount >= _amount,
        "No collateral balance for asset"
    );
    _withdrawCollateral(
        collateral,
        _collateralAddress,
        _amount,
        _tokenId,
        _recipient
    );
    collateral._amount -= _amount;
    emit CollateralWithdrawn(_collateralAddress, _amount, _tokenId, _recipient);
}

AND
Update the withdraw Function:
Add a uint256 _tokenId parameter to the withdraw function and propagate it to _withdrawCollateral and any relevant logic.

mapping(address => mapping(uint256 => Collateral)) collateralBalances;