code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Pair.sol#L14 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Lendgine.sol#L14 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/LendgineRouter.sol#L16 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/Payment.sol#L7 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/SwapHelper.sol#L9

Vulnerability details

Impact

Solmate's SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist (yet).

Proof of Concept

This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/1b3adf677e7e383cc684b5d5bd441da86bf4bf1c/src/utils/SafeTransferLib.sol#L9

File: src/periphery/Payment.sol

  import { SafeTransferLib } from "./../libraries/SafeTransferLib.sol";
  SafeTransferLib.safeTransferETH(recipient, balanceWETH);
  SafeTransferLib.safeTransfer(token, recipient, balanceToken);
if (address(this).balance > 0) SafeTransferLib.safeTransferETH(msg.sender, address(this).balance);
  SafeTransferLib.safeTransfer(weth, recipient, value);
  SafeTransferLib.safeTransfer(token, recipient, value);
  SafeTransferLib.safeTransferFrom(token, payer, recipient, value);

File: src/core/Pair.sol

  import { SafeTransferLib } from "../libraries/SafeTransferLib.sol";
if (amount0 > 0) SafeTransferLib.safeTransfer(token0, to, amount0);
if (amount1 > 0) SafeTransferLib.safeTransfer(token1, to, amount1);
if (amount0Out > 0) SafeTransferLib.safeTransfer(token0, to, amount0Out);
if (amount1Out > 0) SafeTransferLib.safeTransfer(token1, to, amount1Out);

File: src/periphery/LendgineRouter.sol

import { SafeTransferLib } from "../libraries/SafeTransferLib.sol";
SafeTransferLib.safeTransfer(decoded.token1, msg.sender, amount1);
SafeTransferLib.safeTransfer(lendgine, params.recipient, shares);
SafeTransferLib.safeTransfer(decoded.token1, msg.sender, amount1);
SafeTransferLib.safeTransfer(decoded.token1, decoded.recipient, collateralOut);
SafeTransferLib.safeTransferFrom(lendgine, msg.sender, lendgine, params.shares);

File: src/core/Lendgine.sol

import { SafeTransferLib } from "../libraries/SafeTransferLib.sol";

SafeTransferLib.safeTransfer(token1, to, collateral); // optimistically transfer SafeTransferLib.safeTransfer(token1, to, collateral);

File: src/periphery/SwapHelper.sol

  import { SafeTransferLib } from "../libraries/SafeTransferLib.sol";
SafeTransferLib.safeTransfer(tokenIn, msg.sender, amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta));
  SafeTransferLib.safeTransfer(params.tokenIn, pair, amountIn);

Tools Used

Manual Review

Recommended Mitigation Steps

Add a contract exist control in functions;

pragma solidity >= 0.8.0;

function isContract(address _addr) private returns(bool isContract) {
       isContract = _addr.code.length > 0;
}

berndartmueller commented 1 year ago

SafeTransferLib is not the same as the Solmate implementation. However, the check for the contract existence is missing as well. Nevertheless, it's the user's responsibility to make sure the interacted tokens are trustworthy. Hence, I'm considering downgrading to QA (Low)

Leaving open for sponsor review.

c4-judge commented 1 year ago

berndartmueller marked the issue as duplicate of #141