code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L132

Vulnerability details

Description

Owner can set new marketplace adapters using setMarketplace:

function setMarketplace(
    bytes32 id,
    address marketplace,
    address adapter,
    address operator,
    bool paused
) external override onlyOwner {
    _marketplaces[id] = DataTypes.Marketplace(
        marketplace,
        adapter,
        operator,
        paused
    );
    emit MarketplaceUpdated(id, marketplace, adapter, operator, paused);
}

Later, marketplace adapters are called via delegatecall in MarketPlaceLogic's _buyWithCredit:

// delegateCall to avoid extra token transfer
Address.functionDelegateCall(
    params.marketplace.adapter,
    abi.encodeWithSelector(
        IMarketplace.matchAskWithTakerBid.selector,
        params.marketplace.marketplace,
        params.payload,
        priceEth
    )
);

The combination of these two functions allow owner to immediate execute any code in the context of the pool. They will set a new marketplace, with malicious contract as adapter, and execute their malicious code in matchAskWithTakerBid.

Conceretely, they may rug all NTokens using these functions:

function mint(
    address onBehalfOf,
    DataTypes.ERC721SupplyParams[] calldata tokenData
) external virtual override onlyPool nonReentrant returns (uint64, uint64) {
    return _mintMultiple(onBehalfOf, tokenData);
}
/// @inheritdoc INToken
function burn(
    address from,
    address receiverOfUnderlying,
    uint256[] calldata tokenIds
) external virtual override onlyPool nonReentrant returns (uint64, uint64) {
    return _burn(from, receiverOfUnderlying, tokenIds);
}
function _burn(
    address from,
    address receiverOfUnderlying,
    uint256[] calldata tokenIds
) internal returns (uint64, uint64) {
    (
        uint64 oldCollateralizedBalance,
        uint64 newCollateralizedBalance
    ) = _burnMultiple(from, tokenIds);
    if (receiverOfUnderlying != address(this)) {
        for (uint256 index = 0; index < tokenIds.length; index++) {
            IERC721(_underlyingAsset).safeTransferFrom(
                address(this),
                receiverOfUnderlying,
                tokenIds[index]
            );
        }
    }
    return (oldCollateralizedBalance, newCollateralizedBalance);
}
/// @inheritdoc INToken
function transferOnLiquidation(
    address from,
    address to,
    uint256 value
) external onlyPool nonReentrant {
    _transfer(from, to, value, false);
}

These functions, guarded by onlyPool, will allow compromised owner to transfer underlying NFTs or mint NTokens for their own gain, making massive profits.

Impact

Owner has complete control of NTokens using unsafe marketplace delegatecall

Proof of Concept

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding timelocks on insertion of new marketplaces, or to not use delegatecall to enter marketplace adapter contracts.

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #54