Holders unable to claim accurate revenue

[c4-bot-10]

Lines of code

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

Vulnerability details

Description

The liquidity infrastructure calculates holder revenue based on the token balance held by each holder and the revenue generated by the controlled NFTs. This revenue is then distributed among holders through the LiquidInfrastructureERC20 contract.

uint256 entitlement = erc20EntitlementPerUnit[j] * this.balanceOf(recipient);

The current method of calculating erc20EntitlementPerUnit in _beginDistribution() introduces inaccuracies because of Solidity's integer math. Specifically, dividing the ERC20 token balance by the total supply in this manner truncates any decimals, leading to a loss of precision.

uint256 supply = this.totalSupply();
for (uint i = 0; i < distributableERC20s.length; i++) {
    uint256 balance = IERC20(distributableERC20s[i]).balanceOf(
        address(this)
    );
    uint256 entitlement = balance / supply;
    erc20EntitlementPerUnit.push(entitlement);
}

Example:

If the balance is 9e18 and the supply is 1e19, the correct entitlement should be 0.9 but the code erroneously calculates it as 0 because the division discards the decimal part.

Impact

• Holders are unable to claim actual revenue and loss tokens in that distribution round.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

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

contract USDT is ERC20{
    constructor() ERC20("USDT mock", "USDT") {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }
}

contract AltheaPOC is Test{

    LiquidInfrastructureNFT private nft;
    LiquidInfrastructureERC20 private erc20;
    USDT private usdt;

    // Accounts
    address public NFT_OWNER = makeAddr("NFT_OWNER");
    address public ERC20_OWNER = makeAddr("ERC20_OWNER");
    address public ALICE = makeAddr("ALICE");
    address public BOB = makeAddr("BOB");

    // Values
    uint256 public ALICE_HOLD_AMOUNT = 100e18;
    uint256 public BOB_HOLD_AMOUNT = 300e18;

    function setUp() public {
        // Deploy USDT mock.
        usdt = new USDT();

        // Deploy LiquidInfrastructureNFT and set thresholds.
        vm.startPrank(NFT_OWNER);
        nft = new LiquidInfrastructureNFT("TestAccount");
        address[] memory erc20s = new address[](1);
        uint256[] memory erc20sAmounts = new uint256[](1);
        erc20s[0] = address(usdt);
        erc20sAmounts[0] = 100e18;
        nft.setThresholds(erc20s, erc20sAmounts);
        usdt.mint(address(nft), 50e18);
        vm.stopPrank();

        // Deploy LiquidInfrastructureERC20.
        vm.prank(ERC20_OWNER);
        erc20 = new LiquidInfrastructureERC20("Test Token", "TEST", new address[](0), new address[](0), 100, new address[](0));

        // Take control of NFT, set distributable ERC20s, Mint token for holders and withdraw nft balances.
        vm.startPrank(NFT_OWNER);
        nft.safeTransferFrom(NFT_OWNER, address(erc20), nft.AccountId());
        vm.stopPrank();

        vm.prank(ERC20_OWNER);
        erc20.addManagedNFT(address(nft));

        address[] memory distributableErc20s = new address[](1);
        distributableErc20s[0] = address(usdt);
        vm.startPrank(ERC20_OWNER);
        erc20.setDistributableERC20s(distributableErc20s);
        erc20.approveHolder(ALICE);
        erc20.approveHolder(BOB);
        erc20.mint(ALICE, ALICE_HOLD_AMOUNT);
        erc20.mint(BOB, BOB_HOLD_AMOUNT);
        erc20.withdrawFromAllManagedNFTs();
        assertEq(usdt.balanceOf(address(erc20)), 50e18);
        vm.stopPrank();
    }

    function test_entitlementCalculationRoundingIssue() public {
        uint256 entitlement = usdt.balanceOf(address(erc20)) * 1e18 / erc20.totalSupply();
        uint256 aliceExpectedRevenue = ALICE_HOLD_AMOUNT * entitlement / 1e18;
        uint256 bobExpectedRevenue = BOB_HOLD_AMOUNT * entitlement / 1e18;
        console.log("ALICE expected revenue: ", aliceExpectedRevenue);
        console.log("BOB expected revenue: ", bobExpectedRevenue);

        // Distribute revenue between holders
        vm.startPrank(ERC20_OWNER);
        vm.roll(101);
        erc20.distributeToAllHolders();
        vm.stopPrank();

        console.log("....... After distribution .......");
        console.log("ALICE claimed revenue: ", usdt.balanceOf(ALICE));
        console.log("BOB claimed revenue: ", usdt.balanceOf(BOB));
        assertEq(usdt.balanceOf(ALICE), 125e17);
        assertEq(usdt.balanceOf(BOB), 375e17);
    }
}

POC setup

foundry.toml:

[profile.default]
[profile.default]
src = 'contracts'
out = 'out'
libs = ['node_modules', 'lib']
test = 'test'
cache_path  = 'cache_forge'
remappings = [
    "@openzeppelin/=node_modules/@openzeppelin/",
    "ds-test/=lib/forge-std/lib/ds-test/src/",
    "eth-gas-reporter/=node_modules/eth-gas-reporter/",
    "forge-std/=lib/forge-std/src/",
    "@contracts/=contracts/"
]

forge test --mt test_entitlementCalculationRoundingIssue -vv

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Utilize external libraries like ABDK Math for advanced calculations and fixed-point math functionalities.

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #757

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report

[0xA5DF]

Marking as med, since the case where the result of the division is zero is already reported by the bot. We're left only in the case where it's greater than zero (e.g. 1.9 is rounded to 1), in that case the funds wouldn't be lost permanently, we'd just have to wait for the balance to cross the 1 threshold again (i.e. after rounding 1.9 to 1 and distributing the 1, we're left with a balance that will result in 0.9. We'd just have to wait till it reaches 1 again).

Also, the cases where the rounding is significant are more rare and less likely to happen since we have a more limited range of results where this can happen (let's say a 30% loss is significant, that's limited to [1.3, 2), [2.6, 3], [3.9,4))

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)

[SovaSlava]

I think this essay describes what the bot found. Yes, a slightly different form. But this does not change the essence. If the sponsor corrects the problem found by the bot, then this issue will become invalid.

[shealtielanz]

Hello @0xA5DF This report is a valid Low. I believe that this is a bot report with a POC.

Thank you for your time sir.

[visualbits]

I agree that the bot reported the issue, but it only mentioned that the result would be zero without addressing the precision loss of greater values. I agree with the opinion of @0xA5DF .

[SovaSlava]

Does it change anything? If sponsor will fix bit issue, this issue(described by warden) will be fixed automaticaly

[shealtielanz]

I believe @visualbits is correct based on the statement by the Judge.

We're left only in the case where it's greater than zero (e.g. 1.9 is rounded to 1), in that case the funds wouldn't be lost permanently, we'd just have to wait for the balance to cross the 1 threshold again (i.e. after rounding 1.9 to 1 and distributing the 1, we're left with a balance that will result in 0.9. We'd just have to wait till it reaches 1 again).

[mcgrathcoutinho]

Based on the recent SC ruling here, issues are escalatable if the bot report does not define the appropriate severity, which is true in this case since the bot just mentions one half of the issue and does not describe the full impact this would have on the approved holders. Additionally, as the last line in the ruling mentions, the sponsor has no agency in determining fixes for bot findings.

Furthermore, this issue would also result in delay of revenue distribution, which has been covered in some of the duplicates.

For this reason, I think this clearly deserves a medium-severity.

[0xA5DF]

I'm going to mark this one OOS. It's basically the same issue, the same root cause. It's true that the bot report didn't describe the issue well enough and the mitigation provided there doesn't really resolve the issue. However, I believe that once this issue has surfaced the sponsor would apply an adequate mitigation. It doesn't take a genius to realize that the mitigation isn't suitable and the average researched/dev can easily find out what's the best practice to deal with loss of precision.

For #638 which mentioned the bot report - I'm going to credit this one as a low, for the rest I'm going to mark as OOS.

[k4zanmalay]

@0xA5DF apologies for commenting after PJQA, but shouldn't this report be OOS?

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[0xA5DF]

Yeah, that's weird, the extension marked all dupes OOS but this one Marked this again as OOS