Beedle - Oracle free perpetual lending

Summary

Return values of external calls to ERC20 transfer() and transferFrom() are not checked, so failed transfers will still be accounted for.

Vulnerability Details

In the ERC20 specification transfer() and transferFrom() return bool success, indicating whether the transfer was performed. The specification clarifies:

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

While most tokens revert on a failure condition, some like ZRX return false instead. Lender.sol contains 20 calls to transfer() and transferFrom() where the return value is not checked. Staking.sol contains 2 (ignoring 2 calls to WETH, which is assumed to be the canoncial implementation which does revert on failure).

Impact

Critical. Attackers can drain pools where the collateral token returns false on a failed transfer. Accounting is affected in many other cases. See PoC for a concrete example.

PoC

1. Alice creates a Pool on Ethereum Mainnet with USDT as loan token and ZRX as collateral token. She deposits 1000 USDT.

2. Bob knows that ZRX does not revert but returns false on an unsuccessful transfer. He calls borrow with collateral=10**70 and debt=pool.poolBalance.

3. The external call to transferFrom on the collateralToken (ZRX) returns false because Bob does not have this amount of ZRX. However execution continues despite that.

4. All USDT is transferred to Bob against 0 collateral. Alice can never reclaim her USDT.

Tools Used

Manual Review

Recommendations

Use OpenZeppelin's SafeERC20 library to check the return value and handle other ERC20 edge cases (e.g. USDT, which does not return anything).