Due to rounding down depositors will lost huge amount of share

[hats-bug-reporter[bot]]

Github username: @itsabinashb Twitter username: akatabletos Submission hash (on-chain): 0x182ebb6dcbbdc5c8a0e43164f413a63b900df46aa794984683d9e72fbf152e37 Severity: high

Description: Description\ To deposit in Atom vault user has to call depositAtom(), while depositing the share of user is calculated in getDepositSharesAndFees(), in this function convertToShares() is called where FixedPointMathLib::mulDiv() is called. This mulDiv() uses simple / method which result rounding down issue by which the user losts lots of share.

Attack Scenario\ See the PoC section.

Attachments

1. Proof of Concept (PoC) File\

I explained everything in code comment, please go throught every line of the PoC to understand the issue:\

    function test_roundingDown() public {
        bytes memory uri = abi.encode("subject");
        address user1 = makeAddr("1");
        address user2 = makeAddr("2");
        address user3 = makeAddr("3");
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);
        vm.deal(user3, 100 ether);
        uint256 testAtomCost = getAtomCost();
        vm.startPrank(user1);
        uint256 subjectId = ethMultiVault.createAtom{value: testAtomCost}(uri);
        ethMultiVault.depositAtom{value: 50 ether}(address(user1), subjectId);
        vm.stopPrank();
        vm.prank(user2);
        ethMultiVault.depositAtom{value: 50 ether}(address(user2), subjectId);
        //lets do depositAtom operations for user3 by hand:
        uint msg_value = 50 ether;
        uint protocolFee = ethMultiVault.protocolFeeAmount(msg_value, subjectId);
        uint userDepositAfterprotocolFee = msg_value - protocolFee;
        //Now _deposit() will be called, receiver = user3, id = subjectId, userDepositAfterprotocolFee
        // _deposit(address(user3), subjectId, userDepositAfterprotocolFee)
        // Now, we are inside _deposit(). Lets call the getDepositSharesAndFees():
        // getDepositSharesAndFees(userDepositAfterprotocolFee, subjectId)
        // Inside getDepositSharesAndFees() as the vault is atom so atomDepositFractionAmount is 0
        // so,
        uint256 userAssetsAfterAtomDepositFraction = userDepositAfterprotocolFee - 0;
        (, uint totalShare) = ethMultiVault.vaults(subjectId);
        (,,,,uint minShare,,,) = ethMultiVault.generalConfig();
        uint entryFee;
        if(totalShare == minShare){
            entryFee = 0;
        }else {
            entryFee = ethMultiVault.entryFeeAmount(userDepositAfterprotocolFee, subjectId);
        }
        uint256 userAssetsAfterTotalFees = userAssetsAfterAtomDepositFraction - entryFee;   
        //Now the asset will be converted to share, lets do all calculations by hand
        (uint _totalAssets, uint _totalShare) = ethMultiVault.vaults(subjectId);
        console.log("totalShare; ", _totalShare); // 91698854749990508809
        console.log("totalAssets; ", _totalAssets); // 99000100000000100000
        console.log("asset: ", userAssetsAfterTotalFees); // 47025000000000000000
        /**
        here, to check the rounding down issue we directly see the `mulDiv()` operation. Here,
        asset i.e x = userAssetsAfterTotalFees, supply i.e y = _totalShare & z = _totalAssets.
        x = 47025000000000000000
        y = 91698854749990508809
        d = 99000100000000100000
        z = mul(x,y) = 4312138644618303676743225000000000000000
        Now, lets analyse this line of the mulDiv():
        if iszero(mul(or(iszero(x), eq(div(z, x), y)), d))
        => if iszero(mul(or(iszero(47025000000000000000), eq(div(4312138644618303676743225000000000000000,47025000000000000000),91698854749990508809))99000100000000100000)) 
        => if iszero(mul(or(0,eq(91698854749990508809,91698854749990508809)),99000100000000100000)) 
        => if iszero(mul(or(0,1),99000100000000100000))
        => if iszero(mul(1,99000100000000100000));
        => if iszero(99000100000000100000) == false

        so here the 'if' block will not execute, 
        so, z = div(z,d) = 4312138644618303676743225000000000000000 / 99000100000000100000
         */
    }

     /// @dev Returns `floor(x * y / d)`.
    /// Reverts if `x * y` overflows, or `d` is zero.
    function mulDiv(uint256 x, uint256 y, uint256 d) internal pure returns (uint256 z) {
        /// @solidity memory-safe-assembly
        assembly {
            z := mul(x, y)
            // Equivalent to `require(d != 0 && (y == 0 || x <= type(uint256).max / y))`.
            if iszero(mul(or(iszero(x), eq(div(z, x), y)), d)) {
                mstore(0x00, 0xad251c27) // `MulDivFailed()`.
                revert(0x1c, 0x04)
            }
            z := div(z, d)
        }
    }

If you run the test in DepositTriple.t.sol you will only see the logs, the bug is explained in comments.\

Now we have z's value which is:\

z = 4312138644618303676743225000000000000000 / 99000100000000100000

This division will revert, let's find out the mod:\

➜ uint num = 4312138644618303676743225000000000000000/99000100000000100000;
Compiler errors:
Error (4486): Type rational_const 3920...(26 digits omitted)...0000 / 90000090909091 is not implicitly convertible to expected type uint256. Try converting to type ufixed256x57 or use an explicit conversion.
  --> ReplContract.sol:29:1:
   |
29 | uint num = 4312138644618303676743225000000000000000/99000100000000100000;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

➜ 4312138644618303676743225000000000000000%99000100000000100000
Type: uint256
├ Hex: 0x1f3baf559e1e99c20
├ Hex (full word): 0x1f3baf559e1e99c20
└ Decimal: 36009363536985300000
➜ !exec cast to-unit 36009363536985300000 ether
36.009363536985300000

You can see the revert from division above, by doing mod we can see this much ether the user will loss due to rounding down issue which is 36 ether.

2. Revised Code File (Optional)

[itsabinashb]

In mulDiv() the mentioned division will not revert, it will simply round down. In chisel it reverted which can be ignored.

[itsabinashb]

Extra PoC:

➜ function mulDiv(uint256 x, uint256 y, uint256 d) internal pure returns (uint256 z) {
        /// @solidity memory-safe-assembly
        assembly {
            z := mul(x, y)
            // Equivalent to `require(d != 0 && (y == 0 || x <= type(uint256).max / y))`.
            if iszero(mul(or(iszero(x), eq(div(z, x), y)), d)) {
                mstore(0x00, 0xad251c27) // `MulDivFailed()`.
                revert(0x1c, 0x04)
            }
            z := div(z, d)
        }
    }
➜ uint z  = mulDiv(47025000000000000000, 91698854749990508809, 99000100000000100000)
➜ z
Type: uint256
├ Hex: 0x25c793cb2b71fea83
├ Hex (full word): 0x25c793cb2b71fea83
└ Decimal: 43556912009364630147             // @audit here we get the result of mulDiv()

➜ 47025000000000000000 * 91698854749990508809
Type: uint256
├ Hex: 0xcac17b1aa1d16f338e21b68b023328000
├ Hex (full word): 0xcac17b1aa1d16f338e21b68b023328000
└ Decimal: 4312138644618303676743225000000000000000      // @audit this is numerator

➜ 99000100000000100000 * 43556912009364630147      
Type: uint256
├ Hex: 0xcac17b1aa1d16f336ee6073564148e3e0
├ Hex (full word): 0xcac17b1aa1d16f336ee6073564148e3e0
└ Decimal: 4312138644618303676707215636463014700000       //@audit this is result: of denominator * result of division from mulDiv()

➜ 4312138644618303676707215636463014700000 + 36009363536985300000    //@audit adding the remainder [from report] with the result 
Type: uint256
├ Hex: 0xcac17b1aa1d16f338e21b68b023328000
├ Hex (full word): 0xcac17b1aa1d16f338e21b68b023328000
└ Decimal: 4312138644618303676743225000000000000000  // @audit we got the numerator
➜ 

So, it was proved that it is rounding down.

[itsabinashb]

@mihailo-maksa May i know why it is invalid?? It is clear rounding down issue with proof of concept.

[itsabinashb]

If the issue was invalidated because the mulDiv() is FixedPointMathLib.sol which is not in scope then it should not be invalidated because the contract should not use such functions which has rounding down issue. And the severity is high because it is a case of fund loss.