The risk of not charging composition fee for tokenY when mint

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L545

Vulnerability details

Impact

The bug is related to 'mint' function in LBPair.sol. Theoretically the clauses on L534 and L545 will not be true at the same time. However, calculation rounding down on L521, L528 and L529 discard some precision, which make the two clauses may be both true. The current implementation will skip composition fee charging for tokenY at this point.

   function mint(
        // ...
    )
    {
       // ...

        unchecked {
            for (uint256 i; i < _ids.length; ++i) {
              
                // ...

                if (_mintInfo.id >= _pair.activeId) {
                    if (_mintInfo.id == _pair.activeId) {
                        uint256 _totalSupply = totalSupply(_mintInfo.id);

                        uint256 _userL = _price.mulShiftRoundDown(_mintInfo.amountX, Constants.SCALE_OFFSET) +
                            _mintInfo.amountY; // @audit: user liquidity rounded down on L521

                        uint256 _receivedX;
                        uint256 _receivedY;
                        {
                            uint256 _supply = _totalSupply + _userL;
                            _receivedX = (_userL * (uint256(_bin.reserveX) + _mintInfo.amountX)) / _supply; // @audit: _receivedX rounded down on L528
                            _receivedY = (_userL * (uint256(_bin.reserveY) + _mintInfo.amountY)) / _supply; // @audit: _receivedY rounded down on L529
                        }

                       // ...

                        if (_mintInfo.amountX > _receivedX) { // @audit: Clause at L534
                           // @audit: charege X token fee
                        } else if (_mintInfo.amountY > _receivedY) { // @audit Clause at L545
                           // @audit: charge Y token fee
                        }
                    } else if (_mintInfo.amountY != 0) revert LBPair__CompositionFactorFlawed(_mintInfo.id);
                } else if (_mintInfo.amountX != 0) revert LBPair__CompositionFactorFlawed(_mintInfo.id);

                // ...
            }
            
           // ...
        }
       // ...
    }

With a specific example：
Let

price = 99 * 2 ** 128 / 100   (that is 0.99)
reserveXOfBin = 100
reserveYOfBin = 100
amountXInForBin = 3
amountYInForBin = 3

Then

supplyOfBin = 100 * 0.99 + 100 = 199
userLiquity = 3 * 0.99 + 3 = 5 (round down)
newSupplyOfBin = 199 + 5 = 204
receivedXForBin = 5 * (100 + 5) / 204 = 2 (round down)
receivedYForBin = 5 * (100 + 5) / 204 = 2 (round down)

We get

(amountXInForBin > receivedXForBin)  && (amountYInForBin > receivedYBin )

Proof of Concept

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L521

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L528

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L529

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L534

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L545

Tools Used

Recommended Mitigation Steps

Change 'if else if' clause to 'if if' clause

   function mint(
        // ...
    )
    {
       // ...

        unchecked {
            for (uint256 i; i < _ids.length; ++i) {
              
                // ...

                if (_mintInfo.id >= _pair.activeId) {
                    if (_mintInfo.id == _pair.activeId) {
                        
                       // ...
                       if (_mintInfo.amountX > _receivedX) { // @autdit: if
                           // ....
                        } 
                       if (_mintInfo.amountY > _receivedY) { // @audit also if
                           // ...
                        }
                    }
                } 

                // ...
            }
            
           // ...
        }
       // ...
    }

[GalloDaSballo]

The Warden has shown how, due to rounding, a loss of fees can happen.

Because the issue arises from a rounding error, in lack of further detail am downgrading to QA.

I have run the base tests with a check to see if both fees should have been paid and wasn't able to reproduce.

I'd recommend a fuzzing test to the sponsor to find all edge cases, alternatively the fix of doing 2 checks may be sufficient.

Because of lack of evidence am downgrading to QA Low, would recommend adding more details next time

L

[c4-judge]

Duplicate of #177