145.md

amaechieth

medium

owner can cause a pool to be permanently in liquidation state

Summary

removeD3Pool lacks validation if a pool is currently being liquidated and will overwrite the previous pool stored here _PENDING_REMOVE_POOL_ = pool. The assets in the overwritten pool will unrecoverable.

Vulnerability Detail

function removeD3Pool(address pool) external onlyOwner {
        require(allPoolAddrMap[pool] == true, Errors.POOL_NOT_ADDED);
        ID3MM(pool).startLiquidation(); 

        allPoolAddrMap[pool] = false;
        _PENDING_REMOVE_POOL_ = pool; 
        address creator = ID3MM(pool)._CREATOR_();
        address[] memory poolList = creatorPoolMap[creator];
        for (uint256 i = 0; i < poolList.length; i++) {
            if (poolList[i] == pool) {
                poolList[i] == poolList[poolList.length - 1]; 
                creatorPoolMap[creator] = poolList;
                creatorPoolMap[creator].pop();
                emit RemovePool(pool);
                break;
            }
        }
    }

This function calls ID3MM(pool).startLiquidation(); which sets isInLiquidation = true;. This is to prevent users from interacting with the pool until the owner has settled the liquidation.

It also sets _PENDING_REMOVE_POOL_ = pool; which can be problematic if there was an existing pool currently being liquidated and finishPoolRemove hasn't yet been called.

function finishPoolRemove() external onlyOwner {
        ID3MM(_PENDING_REMOVE_POOL_).finishLiquidation(); // @audit-info sets liquidation to false
        _PENDING_REMOVE_POOL_ = address(0);
        emit RemovePool(_PENDING_REMOVE_POOL_);
    }

The protocol assumes the owner will call finishPoolRemove before calling removeD3Pool again, however, there is no check to enforce this.

Once removeD3Pool has been called, the relevant pool is set to falseallPoolAddrMap[pool] = false;.

Given the following scenario:

1. Owner calls removeD3Pool passing address poolA
2. Owner calls removeD3Pool passing address poolB
3. Owner realises this mistake and attempts to call removeD3Pool passing address poolA again, however, the call will fail due to this check require(allPoolAddrMap[pool] == true, Errors.POOL_NOT_ADDED);.

At this point the owner will be able to call finishPoolRemove passing address poolB but will never be able to call removeD3Pool passing address poolA again, meaning all assets in poolA will be unrecoverable.

Impact

All assets in poolA will be lost, causing loss of funds to the protocol and users.

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3Vault.sol#L60-L64

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3Vault.sol#L36-L53

Tool used

Manual Review

Recommendation

In removeD3Pool add the following check:

require(_PENDING_REMOVE_POOL_ == address(0))