Users will receive fewer `shares` than intended due to division before multiplication in `LibOrders::increaseSharesOnMatch`

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L47

Vulnerability details

Impact

The LibOrders::increasesSharesOnMatch function determines the number of shares based on the amount of eth and the time until a match occurs:

    function increaseSharesOnMatch(address asset, STypes.Order memory order, MTypes.Match memory matchTotal, uint88 eth) internal {
        AppStorage storage s = appStorage();

        // @dev use the diff to get more time (2159), to prevent overflow at year 2106
        uint32 timeTillMatch = getOffsetTime() - order.creationTime;
        if (timeTillMatch > C.MIN_DURATION) {
            // shares in eth-days
    @>      uint88 shares = eth * (timeTillMatch / 1 days);
            matchTotal.dittoMatchedShares += shares;

            uint256 vault = s.asset[asset].vault;
            s.vaultUser[vault][order.addr].dittoMatchedShares += shares;
        }
    }

But there is a precision loss in the expression uint88 shares = eth * (timeTillMatch / 1 days) due to integer division before multiplication. In Solidity, when dividing integers, the result is truncated towards zero. If timeTillMatch is not a multiple of 1 days (86400 seconds), the division will truncate any remainder leading to a loss of precision in the calculation of shares. The precision loss in shares calculation results in users receiving fewer shares than they are entitled to, based on the time and eth amount.

Proof of Concept

Link to the code: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L47

Let's consider the following scenario:

1. The getOffset function returns: return uint32(block.timestamp - C.STARTING_TIME);. The current block.timestamp is 1712058842 (in seconds, 02.04.2024, 11:54:02 GMT) and the variable C.STARTING_TIME is 1660353637 (13.08.2022).
2. The order creatin time is: Sunday, 17 March 2024 12:00:02. Therefore, the Epoch timestamp value is 1710676802.
3. The timeTillMatch is calculated in the following way: uint32 timeTillMatch = getOffsetTime() - order.creationTime;.
4. The input parameter eth is 1e18.

Now, we can write a test function that shows the difference between the calculated shares when division before multiplication is done and when the division is performed after the multiplication (the correct way).

    function test_precisionLoss() external {
        //Order creation time: Sunday, 17 March 2024 12:00:02 (1710676802)
        //Current blocktimestamp: 1712058842, 02.04.2024, 11:54:02 GMT
        uint32 timeTillMatch = uint32(1712058842 - 1660353637) - uint32(1710676802 - 1660353637);
        uint88 eth = 1e18;
        uint88 shares = eth * (timeTillMatch / 1 days);
        uint88 sharesCorrect = uint88(uint256(eth * timeTillMatch) / 1 days);
        uint88 diff = sharesCorrect - shares;
        assert(shares < sharesCorrect);
        console.log("shares:", shares);
        console.log("sharesCorrect:", sharesCorrect);
        console.log(diff);
    }

Here is a trace of the function execution:

[5306] TestLiOrders::test_precisionLoss()
    ├─ [0] console::log("shares:", 15000000000000000000 [1.5e19]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("sharesCorrect:", 15995833333333333333 [1.599e19]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log(995833333333333333 [9.958e17]) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop] 

We can see that the calculated shares from the protocol are 1.5e19, but the shares actually should be 1.599e19. The users receive fewer shares than it is intended to receive.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To mitigate this issue, the multiplication by eth should be done before the division by 1 days:

    function increaseSharesOnMatch(address asset, STypes.Order memory order, MTypes.Match memory matchTotal, uint88 eth) internal {
        AppStorage storage s = appStorage();

        // @dev use the diff to get more time (2159), to prevent overflow at year 2106
        uint32 timeTillMatch = getOffsetTime() - order.creationTime;
        if (timeTillMatch > C.MIN_DURATION) {
            // shares in eth-days
-           uint88 shares = eth * (timeTillMatch / 1 days);
+           uint88 shares = uint88(uint256(eth * timeTillMatch) / 1 days);
            matchTotal.dittoMatchedShares += shares;

            uint256 vault = s.asset[asset].vault;
            s.vaultUser[vault][order.addr].dittoMatchedShares += shares;
        }
    }

Assessed type

Decimal

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #179

[raymondfam]

See #179.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope