code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L49

Vulnerability details

Description

The ERC20 specification does not demand implementations to revert when the transfer and transferFrom functions fail. They may use the return value to signal the success code. Some tokens, like ZRX, indeed don't revert.

In Collateral deposit() and withdraw() functions, baseToken transferFrom/transfer is used without checking return code:

function deposit(address _recipient, uint256 _amount) external override nonReentrant returns (uint256) {
  uint256 _fee = (_amount * depositFee) / FEE_DENOMINATOR;
  if (depositFee > 0) { require(_fee > 0, "fee = 0"); }
  else { require(_amount > 0, "amount = 0"); }
  // UNSAFE TRANSFERFROM
  baseToken.transferFrom(msg.sender, address(this), _amount);
  uint256 _amountAfterFee = _amount - _fee;
  if (address(depositHook) != address(0)) {
    baseToken.approve(address(depositHook), _fee);
    depositHook.hook(_recipient, _amount, _amountAfterFee);
    baseToken.approve(address(depositHook), 0);
  }
  /// Converts amount after fee from base token units to collateral token units.
  uint256 _collateralMintAmount = (_amountAfterFee * 1e18) / baseTokenDenominator;
  _mint(_recipient, _collateralMintAmount);
  emit Deposit(_recipient, _amountAfterFee, _fee);
  return _collateralMintAmount;
}

baseToken is some ERC20 token chosen to be the underlying collateral of the Collateral.sol ERC20. In the deposit() flow, the impact of not checking transferFrom success is critical. User can mint an infinite amount of Collateral ERC20 tokens. _mint(_recipient, _collateralMintAmount);.

Therefore, Collateral.sol does not work with ERC20 compatible baseTokens, breaking functionality of the contract.

Note: unsafe transfer/transferForm is also used in TokenSender.send() and WithdrawHook.hook() functions.

Impact

Collateral.sol does not work with ERC20 compatible baseTokens, breaking functionality of the contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Use SafeERC20 library from OpenZeppelin when interacting with generic ERC20 tokens.

hansfriese commented 1 year ago

duplicate of #329

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #329