Beedle - Oracle free perpetual lending

Summary

No checks that transfers were successful

Vulnerability Details

The links provided have transfers() and transferFrom() that dont check the success of transfers. Both functions for ERC20 must return bool but none of the cases check if bool is successful. However even if checked some

Without checking that the transfers for the tokens were successful this results in faulty accounting
Transfers may silently fail, some tokens don't return bool as specified by ERC20 by not conforming to standard

Impact

High: If transfers are assumed successful but do not succeed or incorrectly return failures or success when no tokens transferred into protocol or out of protocol this can lead to accounting problems and value losses to users. Consider he examples below

1. Lender.sol line 130 function setPool() -> if transferFrom lender line 152 fails silently the poolBalance is updated to a new higher value when there are no actual tokens that have been transferred into the protocol from lender. This leads o actual poolBalances being incorrect with existing funds in pool less than state value of balances. Lender may lose tokens as they may reduce poolBalance line 157, if (p.poolBalance < currentBalance) { but do not receive their tokens due to failed transfer to them line 159, this implies lender loses tokens and these tokens may be stuck in protocol as accounting is now faulty to be able to retrieve them since removals for lenders are based on poolBalance

2. Adding to pools by lenders may silently fail function addToPool() line 182 resulting in poolBalances updated to higher value when no actual tokens have been received from the lender leading to reverts when poolBalance is at level poolBalance-amount, as amount was never added to the protocol

3. Lenders can lose tokens when they try remove from pool as Lender.sol function removeFromPool() transfer in line 203 of amount tokens they are removing from pool balance may fail silently to transfer to them but line 201 prior the pool balance will be decreased resulting in lost value/tokens for lender

4. Borrowers may borrow a loanToken but may never receive the tokens or the collateral may never make it into the protocol as transfers in Lender.sol line 232 silently fail but protocol values of pool balances, outstanding loans, debt to borrower as updated when there was no token movement

The above are just examples that show the impact of silently failing transfers and apply to the rest of the functions like repay loan where borrower credited for repaying but they never send in value so lenders lose value. Similarly buying loan, sezing loans, refinancing also become faulty leading to incorrect accounting, leakages, loss of value or inapproporiate gains of value for the borrowers, lenders, auction participants etc

Tools Used

Manual Analysis

Recommendations

1. It is recommended to check return values as in the example below is sure only working with ERC20 tokens that truly return true on success of transfers and false on failure, this mitigation wont work with tokens like ZRX which returns false on token success transfers...
   bool success = IERC20(p.loanToken).transferFrom(
   p.lender,
   address(this),
   p.poolBalance - currentBalance
   );
   if(!success) revert Appropriate_Error;

2. To safeguard against weird ERC20 that dont conform to ERC20 specifications makes use of SafeERC20 wrapper; use OpenZeppelin's SafeERC20 library and its safe methods for ERC20 transfers and use the safeTransferFrom and safeTransfer methods. Example below
   using SafeERC20 for IERC20;
   IERC20(p.loanToken).safeTransferFrom(
   p.lender,
   address(this),
   p.poolBalance - currentBalance
   );

3. It may be prudent to go further and whitelist the allowable tokens for loanToken and collateralToken to ensure they are not problematic ERC20 tokens that may cause problems especially silently failing on transfers