Beedle - Oracle free perpetual lending

Summary

ERC20 token transfers are never checked for success/failure

Vulnerability Details

In almost all (if not all) files the functions transfer and transferFrom are used for transferring ERC20 tokens, but their success/failure is not checked. The issue stems from the fact that some ERC20 tokens return true when the transfer is successful and false when it is not, while others return true on successful transfer, but revert if the transfer is not successful. Since the protocol works with arbitrary ERC20 tokens, it is not possible to know how exactly their transfer functions are implemented.

Impact

ERC20 token transfer are used in many places across the contracts. There are some places where not performing a status check on the transfer can be disastrous for the protocol. Only one example of disastrous place to miss the status check of the transfer will be given to underline the severity of the issue. The fix should add status checks right after transfer and transferFrom are used across the whole audit scope.

Example:

In Staking.sol inside the function deposit the status of transferFrom is not checked. Let's assume the depositor does not have the _amount quantity of the ERC20 token he wants to deposit into the contract. Let's assume the token he is trying to deposit returns true on successful transfer and false otherwise. Because the user does not have enough of the token, transferFrom fails and returns false. This value is not checked and the function call updateFor(msg.sender); and balances[msg.sender] += _amount; are executed. As a result the whole recordkeeping of the contract is broken, because no tokens were transferred, but _amount quantity of the ERC20 token are tracked as deposited on the line balances[msg.sender] += _amount;

Tools Used

Manual review

Recommendations

Check the success/failure status of each transfer (this inclueds transfer and transferFrom) right after the transfer is performed.

Example fix for the issue in the example above:

require(TKN.transferFrom(msg.sender, address(this), _amount), "Transfer failed");

All other places where the status of the transfers is not checked can be fixed in the same way - using require.