# Peapods Update Security Review // 09-03-2024

### Scope
https://github.com/peapodsfinance/contracts/pull/34/files

### Commit hash and branch
[677606deaafbb8651f26473c88994c131560e413](https://github.com/peapodsfinance/contracts/pull/34/commits/677606deaafbb8651f26473c88994c131560e413)

## [M-1] If the pod has transfer tax then calling `manualProcessFee` is impossible

### Summary
`manualProcessFee` function would not work if the pod has transfer tax

### Vulnerability Details

When `manualProcessFee` called by anyone it calls the internal `_transfer` function

    function manualProcessFee(uint256 _slip) external {
        _transfer(address(this), address(this), 0);
        address _rewards = StakingPoolToken(lpStakingPool).poolRewards();
        ITokenRewards(_rewards).depositFromPairedLpToken(
          0,
          _slip > 50 ? 50 : _slip // 5% max
        );
      }

Since the `_amount` passed is 0 the `_fee` will be "0". However, if the `_fee` is "0" the if logic will set it to "1". Hence, the last transfer will fail because `_fee` (1) > `_amount` (0)

function _transfer(
    address _from,
    address _to,
    uint256 _amount
  ) internal virtual override {
    .
    .
    uint256 _fee;
    if (_swapping == 0 && _swapAndFeeOn == 1) {
      .
      .
      if (config.hasTransferTax && !_buy && !_sell) {
        _fee = _amount / 10000; // 0.01%
        -> _fee = _fee == 0 ? 1 : _fee;
        super._transfer(_from, address(this), _fee);
      }
    }
    .
    // dev: _amount = 0, _fee = 1 ===== underflow
    -> super._transfer(_from, _to, _amount - _fee);
  }

### Links to code
https://github.com/peapodsfinance/contracts/blob/7caaf1d848def38f4060d934bdf70edd5b56c52f/contracts/DecentralizedIndex.sol#L317-L324

https://github.com/peapodsfinance/contracts/blob/7caaf1d848def38f4060d934bdf70edd5b56c52f/contracts/DecentralizedIndex.sol#L161-L169
### Impact
Mislogic
### Recommendation
`_fee = _fee == 0 ? 1 : _fee;`
this logic inside the `_transfer` function can be removed. If the fee to be taken is too less, then maybe it is considerable to not take the fee at all.

- [ ] Fixed
- [ ] Will not fix

## [M-2] `setPartner` can be used to harm the pod

### Summary
Partner can set the such new partner address that would harm the pod

### Vulnerability Details
Partner can set a new partner address by calling this function:

function setPartner(address _partner) external onlyPartner {
    config.partner = _partner;
  }

However, if the partner sets this to say an address that is blacklisted, then the entire pod transactions would fail

Potentially setting it to V2_ROUTER, address(this) and V2_POOL would also make things complicated.

### Links to code
https://github.com/peapodsfinance/contracts/blob/7caaf1d848def38f4060d934bdf70edd5b56c52f/contracts/DecentralizedIndex.sol#L434-L436
### Impact
Partner can harm the pod
### Recommendation
Don't let partner to set new partner address to addresses that would harm the pod.

Few addresses I think should be restricted for partner to transfer the partner address:
**1- blacklist addresses**
**2- V2_POOL**
**3- address(this)**

- [ ] Fixed
- [ ] Will not fix

## [M-3] If the pods underlying token/s is an another pod with transfer tax, then bonds would not work

### Summary
If the pods underlying token has an another pTKN with transfer tax, the transfer validation would fail hence the core logic would not work.

### Vulnerability Details
When users bond this is the part where the transfer from the user to pod happens:

function bond(
    address _token,
    uint256 _amount,
    uint256 _amountMintMin
  ) external override lock noSwapOrFee {
    .
    .
    .
    for (uint256 _i; _i < indexTokens.length; _i++) {
      uint256 _transferAmt = _firstIn
        ? getInitialAmount(_token, _amount, indexTokens[_i].token)
        : (IERC20(indexTokens[_i].token).balanceOf(address(this)) *
          _tokenAmtSupplyRatioX96) / FixedPoint96.Q96;
      -> _transferFromAndValidate(
        IERC20(indexTokens[_i].token),
        _msgSender(),
        _transferAmt
      );
    }
    .
  }

Say that the `_transferAmt` calculated as "10" and the underlying token is an another pTKN that has transfer tax of 10% (for simplicity let's say 10%).

`_transferFromAndValidate` will fail because when the "10" tokens are transferred from the user to the destination pod, since there is a 10% transfer tax, the "1" of those tokens will be sent to the pTKN that is being transfered. Hence, only 9 tokens will be received by the destination pod. The require statement inside will revert as we can observe in below code snippet:

function _transferFromAndValidate(
    IERC20 _token,
    address _sender,
    uint256 _amount // dev: 10
  ) internal {
    // dev: balance before = 0
    uint256 _balanceBefore = _token.balanceOf(address(this));
    // dev: 1 sent to the pod, so only 9 actually transferred
    _token.safeTransferFrom(_sender, address(this), _amount);
    // dev: 9 >= 0 + 10 ? REVERT
    -> require(
      _token.balanceOf(address(this)) >= _balanceBefore + _amount,
      'TFRVAL'
    );
  }

### Links to code
https://github.com/peapodsfinance/contracts/blob/7caaf1d848def38f4060d934bdf70edd5b56c52f/contracts/WeightedIndex.sol#L119-L163

https://github.com/peapodsfinance/contracts/blob/7caaf1d848def38f4060d934bdf70edd5b56c52f/contracts/DecentralizedIndex.sol#L249-L260
### Impact
Mislogic
### Recommendation
I guess you can not use the `_transferFromAndValidate`. I don't see that it brings any additional security, and also debond not uses it but only bond. So my suggestion would be to remove `_transferFromAndValidate` completely from contracts code and use the safeTransfer

- [ ] Fixed
- [ ] Will not fix

### Low Severity Issues and Best Practices

* Modify this function to support new reward tokens

    function manualProcessFee(uint256 _slip) external {
        _transfer(address(this), address(this), 0);
        address _rewards = StakingPoolToken(lpStakingPool).poolRewards();
        ITokenRewards(_rewards).depositFromPairedLpToken(
          0,
          _slip > 50 ? 50 : _slip // 5% max
        );
      }

- [ ] Fixed
- [ ] Will not fix