Commit hash link:
https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0

Audited by: Parth Patel (Parth#7949)

# Issues
**[H-1]** The product of reserves to be used for constant product formula is calculated wrongly.
On [line 111-121](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L111-L121), SpaceLP.sol has following code:

        reserveEth = address(this).balance;
        reserveSpc = coin.balanceOf(address(this));
        kLast = reserveEth * reserveSpc;

        _burn(address(this), liquidity);
        bool success;
        success = coin.transfer(to, sendSpc);
        require(success, "coin transfer failed");

        (success, ) = address(to).call{value: sendEth}("");
        require(success, "eth transfer failed");

The above code calculates the reserve and assign the product of two reserves in `kLast`. But after calculating the reserve, it sends both `SPC` and `ETH` to the receipient who removes liquidity. So, the value of `reserveEth` and `reserveSpc` in the Pool will be less because it is sent. The wrong calculation of `kLast` will result in `swap` function to behave incorrectly as it is used to calculate amount of `SPC` needed to swap for `ETH` and vice-versa. The vulnerable lines are [Line 145](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L145) and [Line 157](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L157).

Consider: updating the reserves after transfer is made. As there is reentrnacy lock used here, there won't be an issue of reentrancy vulnerability. If the lock was not to be used, then update the reserve before making transfer by deducting the amount to be sent and use checks-effects-interactions pattern.

Fixed code:

        reserveEth = address(this).balance - sendEth;
        reserveSpc = coin.balanceOf(address(this)) - sendSpc;
        kLast = reserveEth * reserveSpc;

        _burn(address(this), liquidity);
        bool success;
        success = coin.transfer(to, sendSpc);
        require(success, "coin transfer failed");

        (success, ) = address(to).call{value: sendEth}("");
        require(success, "eth transfer failed");

**[H-2]** `addLiquidity` function doesn't take care of more SPC send, SPC transfer tax, unsynced reserves and doesn't refund excess ETH to sender
On [line 27-72](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceRouter.sol#L27-L72) in `addLiquidity` function, SpaceRouter.sol has following code:

        function addLiquidity(uint256 spc)
        external
        payable
        returns (uint256, uint256)
    {
        uint256 spcAmount = spc;
        uint256 ethAmount = msg.value;

        require(
            spcAmount > 0 && ethAmount > 0,
            "spcAmount > 0 && ethAmount > 0"
        );

        (uint256 reserveSpc, uint256 reserveEth) = lp.getReserves();

        uint256 spcToTransfer;
        uint256 ethToTransfer;

        if (reserveSpc == 0 && reserveEth == 0) {
            spcToTransfer = spcAmount;
            ethToTransfer = ethAmount;
        } else {
            uint256 optimalSpc = (spc * (reserveEth)) / reserveSpc;
            if (optimalSpc <= spcAmount) {
                (spcToTransfer, ethToTransfer) = (ethAmount, optimalSpc);
            } else {
                uint256 optimalEth = (ethAmount * (reserveSpc)) / reserveEth;
                if (optimalEth <= ethAmount) {
                    (spcToTransfer, ethToTransfer) = (optimalEth, spcAmount);
                }
            }
        }

        if (msg.value > ethToTransfer) {
            ethToTransfer = msg.value;
        }

        bool success;
        success = coin.transferFrom(msg.sender, address(lp), spcToTransfer);
        require(success, "coin transfer failed");

        // mint LP tokens
        lp.deposit{value: ethToTransfer}(msg.sender);

        return (spcToTransfer, ethToTransfer);
    }

The above function has following vulnerabilities:
- It handles the case where excess `ETH` is sent by the lp provider compared to the `SPC` but doesn't handle the case where `SPC` provided is more compared to `ETH`.
- While calculation of required ETH, it doesn't take into account `SPC` transfer tax.
- If the reserves are not synced and there is some additional `ETH` or `SPC` in the pool, then the above function doesn't benefit liquidity provider by considering that amount.
- The above function doesn't refund to users additional `ETH` sent.
- It fetches all `SPC` which is not the desired behaviour if `SPC` is more compared to required.
- `spcToTransfer` and `ethToTransfer` are not assigned correct values.

Consider: Handling the above cases appropriately.

**[Q-1]** Locks used in all functions can be avoided using checks-effects-interactions pattern.

All the function in `SpaceLP.sol` and `SpaceRouter.sol` uses locks to prevent reentrancy attack. Using locks increases gas usage as it writes to storage variable during each function call. It could have been avoided using checks-effects-interactions pattern.

**[Q-2]** Calculation of `liquidity` when `totalLp` is zero can revert.
On [line 75-77](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L75-77), SpaceLP.sol has following code:

        if (totalLp == 0) {
            liquidity = sqrt(diffSpc * diffEth) - MINIMUM_LIQUIDITY;
            _mint(address(burner), MINIMUM_LIQUIDITY);
        }

Calculation of liquidity can revert if `sqrt(diffSpc * diffEth)` is less than `MINIMUM_LIQUIDITY`.

Consider: adding a check if `sqrt(diffSpc * diffEth) >= MINIMUM_LIQUIDITY`.

Fixed code:

        if (totalLp == 0) {
            require(sqrt(diffSpc * diffEth) >= MINIMUM_LIQUIDITY, "insufficient liquidity");
            liquidity = sqrt(diffSpc * diffEth) - MINIMUM_LIQUIDITY;
            _mint(address(burner), MINIMUM_LIQUIDITY);
        }

**[Q-3]** Missing check of `liquidity` more than 0.
On [line 90](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L90), SpaceLP.sol has following code:

        _mint(to, liquidity);

This doesn't check if `liquidity` to be provided is more than zero. If it is equal to zero, lp provider won't get any LP tokens and should revert/

Consider: adding a check if `liquidity > 0`.

Fixed code:

        require(liquidity > 0, "insufficient liquidity");
        _mint(to, liquidity);

**[Q-4]** Remove unused imports and unneeded comments.
On [line 7](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L7) and [line 60](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L60), SpaceLP.sol has following code:

        //line 7
        _mint(to, liquidity);

        //line 60
        // _mint(to, 100);

Consider: removing unused imports and comments.

**[Q-5]** Burner address is preferred to be a address whose private key is not known.
On [line 22-23](https://github.com/0xMacro/student.patnir/blob/8417ccb38ace197a9934187e67ccfe97d243c4c0/lp/contracts/SpaceLP.sol#L22-L23), SpaceLP.sol has following code:

       address public immutable burner =
        0xC167ac038A6DD5181685F70891de759C29F0d258;

Burner address is used so that pool doesn't face issues with zero liquidity. It is preferred to use a address whose private key is not known. Using a known address opens a chance for hack which can ruin the usage of burner address. Also, it can be declared as `constant` instead of `immutable` because it is assigned at compiled time.