code-423n4 / 2024-02-althea-liquid-infrastructure-findings

3 stars 1 forks source link

Malicious NFT account actors can still withdraw the contracts ERC20 balances after transferring ownership of the NFT to the LiquidInfrastructureERC20 #741

Closed c4-bot-3 closed 9 months ago

c4-bot-3 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureNFT.sol#L158

Vulnerability details

Impact

Malicious NFT account actors can still withdraw the contracts ERC20 balances after transferring ownership of the NFT to the LiquidInfrastructureERC20

Proof of Concept

The protocol enforces that contracts must own the nftContract to withdraw the contracts ERC20 balances

this contract must already be the owner of the nftContract

This invariant is also checked in the addManagedNFT():

function addManagedNFT(address nftContract) public onlyOwner {
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        address nftOwner = nft.ownerOf(nft.AccountId());
@>      require(
            nftOwner == address(this),
            "this contract does not own the new ManagedNFT"
        ); @note
        ManagedNFTs.push(nftContract);
        emit AddManagedNFT(nftContract);
    }

Notice it doesn't accept through approvals but only ownership. However, in the LiquidInfrastructureNFT.sol contract, the withdrawBalancesTo() function would still execute if the caller doesn't own the NFT but still approved:

function withdrawBalancesTo(
        address[] calldata erc20s,
        address destination
    ) public virtual {
        require(
@>          _isApprovedOrOwner(_msgSender(), AccountId),
            "caller is not the owner of the Account token and is not approved either"
        );
        _withdrawBalancesTo(erc20s, destination);
    }

Now, let's consider this scenario

Recommended Mitigation Steps

Consider changing withdrawBalancesTo to only allow the owner of the NFT contract to withdraw the contracts ERC20 balances

Assessed type

Access Control

C4-Staff commented 9 months ago

Closing this issue as the warden data are missing.