Beedle - Oracle free perpetual lending

Summary

This contract does not do any checks on whether the IERC20.transferFrom() and IERC20.transfer() functions are successful. ERC20 standard dictates that ERC20 tokens do not revert on failed transfers, rather they return a boolean. While many "ERC20" tokens do revert on failed transfers, the actual spec dictates to return a boolean and thus, many tokens that truly follow the standard will not revert but return false. Lender.sol does not check return values for transferFroms/transfers. Thus a malicious lender could set up a pool using a token strictly adhering to ERC20, submit an update to the setPool() function of lender.sol where p.poolBalance is greater than the currentBalance in storage. Lender.sol then does an unsafe transferFrom the lender to itself for the difference. If the token strictly adheres to the standard, a malicious lender could submit a txn knowing they dont have the assets to make up that difference, but since the return boolean is not checked in anyway, Lender.sol will update the pools mapping as if the transfer succeeded and it received the difference in tokens, even when it did not.

I have found many instances of this in the codebase where the lack of a SafeTransfer could potentially/likely/or almost certainly result in massive losses for the protocol and its users. I have linked all instances I could find in the relevant github links. although the exact method of exploitation for each occurence may differ, all stem from the same root cause.

Vulnerability Details

Smart contracts should never use the base IERC20 transfer() or transferFrom() functions since many ERC20's differ in their adherence to spec.

Impact

Lending.sol will consider the pool in question to have a higher p.poolBalance than it really does. You don't have to look much farther in the codebase to see how this can lose protocol/user funds. the next function in Lender.sol, removeFromPool(), uses the stored p.poolBalance to indirectly validate the amount the caller is requesting to remove from the pools via solidity's built in overflow/underflow checks.
_updatePoolBalance(poolId, pools[poolId].poolBalance - amount);
but even if the lender isnt actually entitled to the faulty poolBalance value, since theres no further checks, as long as other users have deposited enough tokens to cover the malicious lenders faulty withdrawal request, it is trivial for the malicious lender to drain the pool of not only his tokens, but all the tokens of every other user in the protocol leading to direct loss of user funds, and potential protocol insolvency.

Tools Used

manual review

Recommendations

Use openZeppelins SafeERC20.sol library for all erc20 transfers, transferfroms, and approvals to prevent these inaccurate modifications to storage.

I would also recommend a line by line review of the codebase to make sure all instances of this vulnerability are found and mitigated.