cyfrin/5-t-swap-audit

5-t-swap-audit's Issues

testing errors

when i use :pragma solidity 0.8.20; i get
Source file requires different compiler version (current compiler is 0.8.19+commit.7dd6d404.Emscripten.clang) - note that nightly builds are considered to be strictly less than the released version

when i change to: pragma solidity >=0.8.19 <0.9.0;

i get an error with my: import {Test} from "forge-std/Test.sol";
Source "forge-std/Test.sol" not found: File import callback not supported

please help

PoC on `sellPoolTokens` to demonstrate its validity as high-risk audit finding

While I'm reviewing the updraft content Smart Contract Security and Auditing on section 5 TSwap part 44 Reporting: sellPoolToken,

    /**
     * @notice wrapper function to facilitate users selling pool tokens in exchange of WETH
     * @param poolTokenAmount amount of pool tokens to sell
     * @return wethAmount amount of WETH received by caller
     */
    function sellPoolTokens(
        uint256 poolTokenAmount
    ) external returns (uint256 wethAmount) {
        // @audit: this is wrong [ according to updraft video walk-through ]
        // should return swapExactInput(minWethToReceive) instead
        return
            swapExactOutput(
                i_poolToken,
                i_wethToken,
                poolTokenAmount,
                uint64(block.timestamp)
            );
    }

there wasn't a demo walk-through for the PoC. Therefore, I'm attempting to write the PoC to complete the reporting for the audit finding on sellPoolToken. However in this attempt, my PoC shows that the original returned function swapExactOutput in sellPoolToken is correct <@@>!?, which is contradicting that this is a high risk audit violation as the rightful returned function is supposed to be swapExactInput according to course content.

I have put down my thought process and validation workflow of the PoC below. Kindly assist to review :

In TSwapPool.t.sol, my test case was written as follow

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import { Test, console } from "forge-std/Test.sol";
import { TSwapPool } from "../../src/PoolFactory.sol";
import { ERC20Mock } from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol";

contract TSwapPoolTest is Test {
    TSwapPool pool;
    ERC20Mock poolToken;
    ERC20Mock weth;

    address liquidityProvider = makeAddr("liquidityProvider");
    address user = makeAddr("user");

    function setUp() public {
        poolToken = new ERC20Mock();
        weth = new ERC20Mock();
        pool = new TSwapPool(address(poolToken), address(weth), "LTokenA", "LA");

        weth.mint(liquidityProvider, 200e18);
        poolToken.mint(liquidityProvider, 200e18);

        weth.mint(user, 10e18);
        poolToken.mint(user, 10e18);
    }

    function testSellPoolToken() public {
        // injecting funds into pool by liquidity provider
        vm.startPrank(liquidityProvider);
        weth.approve(address(pool), 100e18);
        poolToken.approve(address(pool), 100e18);
        pool.deposit(100e18, 100e18, 100e18, uint64(block.timestamp));
        vm.stopPrank();

        // starting pranking that a user want to sell his pool token
        vm.startPrank(user);
        uint256 balanceWethB4Sell = weth.balanceOf(user); // will return 10e18 as minted in setUp
        uint256 balancePoolTokenB4Sell = poolToken.balanceOf(user); // will return 10e18 as minted in setUp
        console.log("balance weth_user before sell: ", balanceWethB4Sell);
        console.log("balance poolToken_user before sell: ", balancePoolTokenB4Sell);

        poolToken.approve(address(pool), 10e18);

        // assuming user only want to sell 1e16 poolToken out of his original poolToken amount of 10e18
        uint256 poolTokenAmountToSell = 1e16;
        console.log("poolTokenAmountToSell: ", poolTokenAmountToSell);

        // Prerequisite: correct back another audit finding ( change 10000 to 1000) on `getInputAmountBasedOnOutput` under the `swapExactOutput`, so that we only test `swapExactOutput` validity in the `sellPoolToken` function and not influenced by the error caused by other factor.
        // as formulated in `getInputAmountBasedOnOutput`, calculatedWethToReceive = ((100e18 * 1e16) * uint256(1000)) / ((100e18 - 1e16) * uint256(997))
        uint256 calculatedWethToReceive = pool.sellPoolTokens(poolTokenAmountToSell);
        console.log("calculatedWethToReceive: ", calculatedWethToReceive);

        // find out how much fee is collected by protocol from the transaction
        // the difference of calculatedWethToReceive and poolTokenAmountToSell will be the fee collected by protocol
        // since formula ((100e18 * 1e16) * uint256(1000)) / ((100e18 - 1e16) * uint256(997)) returns weth that includes fee 
        uint256 feeChargedByProtocol = calculatedWethToReceive - poolTokenAmountToSell;
        console.log("feeChargedByProtocol: ", feeChargedByProtocol);

        uint256 balanceWethAfterSell = weth.balanceOf(user);
        uint256 balancePoolTokenAfterSell = poolToken.balanceOf(user);

        // tapout the expected balance of weth and poolToken after the sell
        // factoring in the fee charged by protocol for the execution of the transaction
        uint256 expectedBalanceWethAftSell = balanceWethB4Sell + calculatedWethToReceive - feeChargedByProtocol;
        uint256 expectedBalancePoolTokenAftSell = balancePoolTokenB4Sell - poolTokenAmountToSell - feeChargedByProtocol;

        console.log("balance weth_user after sell: ", balanceWethAfterSell);
        console.log("expectedBalanceWethAftSell: ", expectedBalanceWethAftSell);

        console.log("balance poolToken_user after sell: ", balancePoolTokenAfterSell);
        console.log("expectedPoolTokenBalanceAftSell: ", expectedBalancePoolTokenAftSell);

        console.log("poolTokenBalanceIfNoFeeImposed: ", balancePoolTokenB4Sell - poolTokenAmountToSell);

        assertEq(balanceWethAfterSell, expectedBalanceWethAftSell);
        assertEq(balancePoolTokenAfterSell, expectedBalancePoolTokenAftSell);
    }
}

After this test, if returned function swapExactOutput in sellPoolToken is not correct, then this test should fail, indicating that swapExactOutput is not the rightful returned function for sellPoolToken, which then considered as a high risk vulnerability.

However, this test indeed passed. The logs printed via console shown as below:

    balance weth_user before sell:       10000000000000000000
    balance poolToken_user before sell:  10000000000000000000
    poolTokenAmountToSell:                  10000000000000000
    calculatedWethToReceive:                10031093380150452
    feeChargedByProtocol:                      31093380150452
    balance weth_user after sell:        10010000000000000000
    expectedBalanceWethAftSell:          10010000000000000000
    balance poolToken_user after sell:    9989968906619849548
    expectedPoolTokenBalanceAftSell:      9989968906619849548
    poolTokenBalanceIfNoFeeImposed:       9990000000000000000

From the values in the logs, we can see that the final weth in user account ( balance weth_user after sell ) did increase to the exact output the user want as the function does/named swapExactOutput ( increased by exactly 1e16 in the test case ) and the corresponding poolToken did decrease from its initial balance (from 10e18 to 9989968906619849548 ) after deduction of the fee charged by the protocol.

The whole transaction by the protocol only incurred 1 time fee charge, which deducted from the poolToken and keep the final returned weth to the output increase by exactly 1e16 as specified upon executing the sellPoolToken via its parameter poolTokenAmount. Therefore, the returned function swapExactOutput under sellPoolToken seems correct, and not an audit risk.. and the TSwap protocol does charge one time fee only ( as compared to conventional order book system ), which tally to state why TSwap is favorable as it's decentralized and cost saving.

I could have misunderstood / misinterprete how this PoC should be written. As there is no demo-PoC to show that this is a high risk vulnerability, I couldn't find the right reasoning how the course content relates this sellPoolToken as a high risk audit finding if using my own PoC above. May I check with the expert here for further comments or explanation?

Please don't hesitate to highlight any mistakes above as I'm still learning and exploring the topic. Feedbacks are greatly welcomed. Thanks.

Doubt in the math to calculate the `poolTokensToDeposit` in the `deposit()` function

According to the comments, the math explanation considers this invariant to hold:

 // Our invariant says weth, poolTokens, and liquidity tokens must always have the same ratio after the
            // initial deposit
            // poolTokens / constant(k) = weth
            // weth / constant(k) = liquidityTokens
            // aka...
            // weth / poolTokens = constant(k)

Refer code: https://github.com/Cyfrin/5-t-swap-audit/blob/main/src/TSwapPool.sol#L132

But for a constant product AMM, the invariant formula is
poolTokens * wEth = constant(k)

Then why is poolTokens / constant(k) = weth being used?

This being said, the formula for calculating poolTokenToDeposit is correct in the getPoolTokensToDepositBasedOnWeth()
https://github.com/Cyfrin/5-t-swap-audit/blob/main/src/TSwapPool.sol#L429

The comment I pointed out seems a bit confusing/contradicting to what is actually being used in getPoolTokensToDepositBasedOnWeth().