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 #743

Closed c4-bot-9 closed 8 months ago

c4-bot-9 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

0xRobocop commented 9 months ago

Invalid, approvals are cleared when transferring a NFT.

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as insufficient quality report

c4-judge commented 8 months ago

0xA5DF marked the issue as unsatisfactory: Invalid