# Peapods CCIP Security Review // 24-02-2024

### Scope

| Type | File | Logic Contracts | Interfaces | Lines | nLines | nSLOC | Comment Lines | Complex. Score | Capabilities |
| ---- | ------ | --------------- | ---------- | ----- | ------ | ----- | ------------- | -------------- | ------------ |
| 📝 | TokenBridge.sol | 1 | **** | 173 | 144 | 131 | 2 | 70 | **<abbr title='Payable Functions'>💰</abbr>** |
| 📝 | TokenRouter.sol | 1 | **** | 57 | 43 | 33 | 2 | 26 | **** |
| 📝 | **Totals** | **2** | **** | **230** | **187** | **164** | **4** | **96** | **<abbr title='Payable Functions'>💰</abbr>** |

### Commit hash and branch
[20f68c7eba6df2dfbd51d6821d034ba91a236a35](https://github.com/peapodsfinance/contracts/commit/20f68c7eba6df2dfbd51d6821d034ba91a236a35)

## [M-1] Fee taken is lesser due to not using Chainlink TokenPools

### Summary
When calculating the fee for receiving a transaction on the bridge, the `getFee` function of the routers is utilized. This function determines the optimal fee needed and also checks for token transfers. If token transfers are found, it adds **an additional amount** to cover the extra message payload. However, since the implementation does not utilize Chainlink token pools, this fee is never taken into account.

### Vulnerability Details
The bridging fee is fetched from the router contract:

function _getMessageFee(
    ICCIPTokenRouter.TokenConfig memory _bridgeConf,
    address _tokenReceiver,
    uint256 _amount
  ) internal view returns (Client.EVM2AnyMessage memory, uint256) {
    Client.EVM2AnyMessage memory _evm2AnyMessage = _buildMsg(
      _bridgeConf,
      _tokenReceiver,
      _amount
    );
    return (
      _evm2AnyMessage,
      -> ccipRouter.getFee(_bridgeConf.targetChain, _evm2AnyMessage)
    );
  }

Then, router calls the Onramps get fee function which is where the actual calculation of fee happens:

function getFee(
    uint64 destinationChainSelector,
    Client.EVM2AnyMessage memory message
  ) external view returns (uint256 fee) {
    if (message.feeToken == address(0)) {
      // For empty feeToken return native quote.
      message.feeToken = address(s_wrappedNative);
    }
    address onRamp = s_onRamps[destinationChainSelector];
    if (onRamp == address(0)) revert UnsupportedDestinationChain(destinationChainSelector);
    -> return IEVM2AnyOnRamp(onRamp).getFee(destinationChainSelector, message);
  }

function getFee(
    uint64 destChainSelector,
    Client.EVM2AnyMessage calldata message
  ) external view returns (uint256 feeTokenAmount) {
    if (destChainSelector != i_destChainSelector) revert InvalidChainSelector(destChainSelector);
    .
    .
    -> if (message.tokenAmounts.length > 0) {
      (premiumFee, tokenTransferGas, tokenTransferBytesOverhead) = _getTokenTransferCost(
        message.feeToken,
        feeTokenPrice,
        message.tokenAmounts
      );
    } else {
      premiumFee = uint256(feeTokenConfig.networkFeeUSDCents) * 1e16;
    }
    .
    uint256 executionCost = executionGasPrice *
      ((gasLimit +
        s_dynamicConfig.destGasOverhead +
        (message.data.length * s_dynamicConfig.destGasPerPayloadByte) +
        tokenTransferGas) * feeTokenConfig.gasMultiplierWeiPerEth);
    .
      dataAvailabilityCost = _getDataAvailabilityCost(
        dataAvailabilityGasPrice,
        message.data.length,
        message.tokenAmounts.length,
        tokenTransferBytesOverhead
      );
    }

    return (premiumFee + executionCost + dataAvailabilityCost) / feeTokenPrice;
  }

As we can see in above snippet, the token transfers costs are not charged since there are no TokenPools used in the Peapods implementation of CCIP.

### Links to code
https://github.com/smartcontractkit/ccip/blob/645b4b060791fa09d6e3e04076250676be786c7c/contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol#L504-L572

https://github.com/peapodsfinance/contracts/blob/fd95aeb85b0eac498b7d4fff19c11f74341c38ff/contracts/ccip/TokenBridge.sol#L90-L94

https://github.com/smartcontractkit/ccip/blob/f026e9d0fa8f12fa8eb320d0cc581c3d4ad5226a/contracts/src/v0.8/ccip/Router.sol#L78C3-L89C4

https://github.com/smartcontractkit/ccip/blob/645b4b060791fa09d6e3e04076250676be786c7c/contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol#L618-L669
### Impact
Fee taken from bridge actions might not compensate the bridging costs. The fees might not be sufficient to attract Chainlink validators to execute the bridging.
https://docs.chain.link/ccip/billing#network-fee
https://twitter.com/ChainLinkGod/status/1747647328385982901
### Recommendation
soon

- [x] Fixed
- [ ] Will not fix

## Low Severity Issues and Best Practices

* Potential reentrancy threat when bridging tokens, although there are no actual threats it is best to make the refund at the end of the function so that the user does not reenters to an another function to take advantage of it
https://github.com/peapodsfinance/contracts/blob/fd95aeb85b0eac498b7d4fff19c11f74341c38ff/contracts/ccip/TokenBridge.sol#L27-L69
**Recommendation**

    function bridgeTokens(
        uint64 _chainSelector,
        address _tokenReceiver,
        address _token,
        uint256 _amount
      ) external payable returns (bytes32 _messageId) {
        .
        .
        require(msg.value >= _nativeFees, 'FEES');
        _messageId = ccipRouter.ccipSend{ value: _nativeFees }(
          _chainSelector,
          _evm2AnyMessage
        );
        
        -> uint256 _refund = msg.value - _nativeFees;
        -> if (_refund > 0) {
        -> (bool _wasRef, ) = payable(_msgSender()).call{ value: _refund }('');
          -> require(_wasRef, 'REFUND');
        }
        .
        .
      }

- [x] Fixed
- [ ] Will not fix

* Add extra protection in `_ccipReceive` function. The receiver will always be the bridge so it is fair enough to add this to make sure the message is indeed valid.
**Recommendation:**

    function _ccipReceive(
        Client.Any2EVMMessage memory _message
      ) internal override {
        TokenTransfer memory _tokenTransferInfo = abi.decode(
          _message.data,
          (TokenTransfer)
        );
        // dev: the receiver always the bridge and bridge is address(this)
        -> require(_bridgeConf.targetBridge == address(this));
        ICCIPTokenRouter.TokenConfig memory _bridgeConf = tokenRouter.getConfig(
          _tokenTransferInfo.targetToken,
          _message.sourceChainSelector
        );
        address _user = _tokenTransferInfo.tokenReceiver;
        address _token = _tokenTransferInfo.targetToken;
        uint256 _amount = _tokenTransferInfo.amount;
        _processOutboundTokens(
          _token,
          _user,
          _amount,
          _bridgeConf.sourceTokenMintBurn
        );
        emit TokensReceived(
          _message.messageId,
          _message.sourceChainSelector,
          _user,
          _token,
          _amount
        );
      }

- [x] Fixed
- [ ] Will not fix
* Add a check that the destination chain is not the "this" chain.
**Recommendation:**

    function bridgeTokens(
        uint64 _chainSelector,
        address _tokenReceiver,
        address _token,
        uint256 _amount
      ) external payable returns (bytes32 _messageId) {
        require(tokenRouter.globalEnabled(), 'GLDISABLED');
        -> require(_chainSelector != block.chainid);
        .
        .
        .
        return _messageId;
      }

- [x] Fixed
- [ ] Will not fix
* Since every bridge can mint or transfer a single token which is the `ccipReceive` message's `targetToken` every bridge can have a storage variable of that and can use to validate whether the target tokens are matches or not.
**Recommendation**

    IRouterClient public ccipRouter;
    ICCIPTokenRouter public tokenRouter;
    -> address public token; 


    function _ccipReceive(
        Client.Any2EVMMessage memory _message
      ) internal override {
        TokenTransfer memory _tokenTransferInfo = abi.decode(
          _message.data,
          (TokenTransfer)
        );
        ICCIPTokenRouter.TokenConfig memory _bridgeConf = tokenRouter.getConfig(
          _tokenTransferInfo.targetToken,
          _message.sourceChainSelector
        );

        -> require(_tokenTransferInfo.targetToken == token);
        .
        .
      }

- [x] Fixed
- [ ] Will not fix

* Lack of emergency and too much trust on Chainlink. If CCIP message fails, the bridgers tokens are stuck forever. There are no functions to recover it. In official chainlink docs it suggests to add some emergency functions using the `messageId`. An example would be a try/catch as shown here:
https://docs.chain.link/ccip/tutorials/programmable-token-transfers-defensive
- [x] Fixed
- [ ] Will not fix