Clever Mocha Pheasant - Missing Escrow Address Storage in Proxy Deployment Function

[sherlock-admin3]

Clever Mocha Pheasant

Medium

Missing Escrow Address Storage in Proxy Deployment Function

Summary

The CollateralManager contract contains a critical vulnerability in its _deployEscrow function where it fails to store newly deployed escrow proxy addresses in the contract's state. The function correctly creates new BeaconProxy instances but only assigns the address to a return variable without updating the _escrows mapping.

This vulnerability fundamentally breaks the collateral management system as it prevents the contract from tracking and accessing deployed escrow contracts. The impact is severe because:

1. Multiple unused escrow contracts could be deployed for the same bid ID since each call with _escrows[_bidId] == 0 creates a new proxy
2. Any subsequent attempts to interact with the escrow through functions like _deposit and _withdraw will fail or interact with the wrong contract since they rely on _escrows[_bidId] to find the correct escrow
3. This could lead to permanent loss of user collateral as assets could be sent to escrow contracts that become inaccessible
4. The contract wastes significant gas through repeated unnecessary proxy deployments

The vulnerability stems from the following code section:

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

function _deployEscrow(uint256 _bidId)
    internal
    virtual
    returns (address proxyAddress_, address borrower_)
{
    proxyAddress_ = _escrows[_bidId];
    borrower_ = tellerV2.getLoanBorrower(_bidId);
    if (proxyAddress_ == address(0)) {
        require(borrower_ != address(0), "Bid does not exist");

        BeaconProxy proxy = new BeaconProxy(
            collateralEscrowBeacon,
            abi.encodeWithSelector(
                ICollateralEscrowV1.initialize.selector,
                _bidId
            )
        );
        proxyAddress_ = address(proxy);
        // Missing: _escrows[_bidId] = proxyAddress_;
    }
}

Recommended mitigation steps

The fix requires updating the _escrows mapping whenever a new proxy is deployed. Here's the corrected implementation:

function _deployEscrow(uint256 _bidId)
    internal
    virtual
    returns (address proxyAddress_, address borrower_)
{
    proxyAddress_ = _escrows[_bidId];
    borrower_ = tellerV2.getLoanBorrower(_bidId);
    if (proxyAddress_ == address(0)) {
        require(borrower_ != address(0), "Bid does not exist");

        BeaconProxy proxy = new BeaconProxy(
            collateralEscrowBeacon,
            abi.encodeWithSelector(
                ICollateralEscrowV1.initialize.selector,
                _bidId
            )
        );
        proxyAddress_ = address(proxy);
        _escrows[_bidId] = proxyAddress_;  // Store the escrow address
    }
}

These changes ensure proper tracking of deployed escrow contracts and maintain the integrity of the collateral management system while preventing potential loss of user funds.