Althea Liquid Infrastructure - Meera's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L198-L237 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L257-L281 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L441-L445

Vulnerability details

Impact

This is NOT a "Centralization Risk" Here, the protocol can reach an invalid state, breaking accounting and the invariant of the distributableERC20s array being synchronized with the erc20EntitlementPerUnit array Accounting being broken means mis-distribution of funds

Proof of Concept

When initially calling LiquidInfrastructureERC20.distribute(), there's a call to _beginDistribution which builds the erc20EntitlementPerUnit array depending on the distributableERC20s array LiquidInfrastructureERC20.sol#L271-L277 While a LockedForDistribution variable is indeed set to true during this distribution (LiquidInfrastructureERC20.sol#L262), there are no checks preventing a call to setDistributableERC20s():

File: LiquidInfrastructureERC20.sol
441:     function setDistributableERC20s(
442:         address[] memory _distributableERC20s
443:     ) public onlyOwner {
444:         distributableERC20s = _distributableERC20s;
445:     }

This means that, when resuming the distribution, the erc20EntitlementPerUnit may mismatch the distributableERC20s, which may lead to mismanaging funds (especially since the balance of the distributableERC20s is so important (LiquidInfrastructureERC20.sol#L272-L274) and can be mismatched in terms of decimals (like sending way too much USDC, being 6 decimals, by it replacing in the array the index of a 18 decimals ERC20 token) making entitlement potentially way too big (LiquidInfrastructureERC20.sol#L275) or even if it happened to be from the same size: the balances from the holders may mismatch witch would distribute funds wrongly.

Tools Used

Manual review

Recommended Mitigation Steps

1. Either consider prohibiting calls to setDistributableERC20s on LockedForDistribution
2. Or consider setting LockedForDistribution to false in setDistributableERC20s so that _beginDistribution gets called again which would delete erc20EntitlementPerUnit and make the distribution start again from a clean slate

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L413 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L382-L383 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L380 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L362-L364 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L359

Vulnerability details

Impact

DOS on withdrawFromManagedNFTs which is a funds leaving the protocol type of function => loss of funds

Proof of Concept

On the first call to withdrawFromManagedNFTs, nextWithdrawal is equal to 0, which makes the WithdrawalStarted event being emitted (LiquidInfrastructureERC20.sol#L362-L364)

After each call, The nextWithdrawal gets incremented depending on the nextWithdrawal + numWithdrawals value or the ManagedNFTs.length depending on which of these is smaller.

If we take a scenario with those starting parameters:

ManagedNFTs.length == 10  
numWithdrawals == 9
nextWithdrawal == 0

Then the final nextWithdrawal will be equal to 9 here

However, the calls to LiquidInfrastructureERC20.releaseManagedNFT aren't protected against the withdrawFromManagedNFTs being ongoing. By definition, this function Transfers a LiquidInfrastructureNFT contract out of the control of this contract

It means that, by releasing 2 ManagedNFTs through LiquidInfrastructureERC20.releaseManagedNFT, we'll have the following state:

ManagedNFTs.length == 8  
nextWithdrawal == 9

Which will effectively make it impossible to reset nextWithdrawal to 0 again by the protocol's means (LiquidInfrastructureERC20.sol#L382-L383)

Why will it be impossible by the protocol itself? For 2 reasons:

1. Notice here that it'll be impossible to add the released NFTs again as their owner would not be LiquidInfrastructureERC20 anymore.

2. Notice also that there aren't any public mint() or safeMint() methods on LiquidInfrastructureNFT: therefore it won't be possible to just mint other NFTs. It'll be required for the protocol to request to the released NFT's owner to send it back to the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

1. Consider prohibiting calls to releaseManagedNFT on nextWithdrawal > 0
2. Consider using an inequality (would work with the above suggestion not being applied):

File: LiquidInfrastructureERC20.sol
- 382:         if (nextWithdrawal == ManagedNFTs.length) {
+ 382:         if (nextWithdrawal >= ManagedNFTs.length) {
383:             nextWithdrawal = 0;

Assessed type

DoS