# Beedle - Oracle free perpetual lending - Findings Report

# Table of contents
- ## [Contest Summary](#contest-summary)
- ## [Results Summary](#results-summary)
- ## High Risk Findings
- ### [H-01. `sellProfits()` is not slippage protected](#H-01)
- ### [H-02. auctionLength not set in the loan ](#H-02)
- ### [H-03. Incorrect setting of the lender while buying loan ](#H-03)
- ### [H-04. Protocol doesn't support tokens not compatible with ERC20 standard](#H-04)
- ## Medium Risk Findings
- ### [M-01. Frontrun issues in the `giveLoan` and `buyLoan` function](#M-01)
- ### [M-02. Borrows can pay higher interest than expected](#M-02)

# <a id='contest-summary'></a>Contest Summary

### Sponsor: BeedleFi

### Dates: Jul 24th, 2023 - Aug 7th, 2023

[See more contest details here](https://www.codehawks.com/contests/clkbo1fa20009jr08nyyf9wbx)

# <a id='results-summary'></a>Results Summary

### Number of findings:
- High: 4
- Medium: 2
- Low: 0

# High Risk Findings

## <a id='H-01'></a>H-01. `sellProfits()` is not slippage protected

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Fees.sol#L38

## Summary
Uniswap swap done in `sellProfits()` can be sandwiched heavily.

## Vulnerability Details
[`sellProfits()`](https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Fees.sol#L38) makes a Uniswap trade and sets `amountOutMinimum` to 0:

ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _profits,
                tokenOut: WETH,
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: amount,
                amountOutMinimum: 0,
                sqrtPriceLimitX96: 0
            });

Thus this trade succeeds even if the amount received from the swap is 0. This allows anyone to sandwich this trade for their own profit resulting in 0 WETH in `sellProfits()` Uniswap trade.

## Impact
Any token sold via `sellProfits()` for WETH will result in a heavy loss.

## Tools Used
Manual

## Recommendations
Add a `_amountOutMinimum` parameter to `sellProfits` which is passed to `amountOutMinimum`. This ensures that the caller has control over the slippage.
## <a id='H-02'></a>H-02. auctionLength not set in the loan

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L522

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/utils/Structs.sol#L54

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L412

## Summary
`loan.auctionLength` is not updated in `giveLoan` and `buyLoan` functions.

## Vulnerability Details
In the `giveLoan` function, when the loan is given to another pool and the loan is updated with new details, `auctionlength` is not updated which has to be updated based on the new pool parameters.

Similarly in the function `buyLoan` when the loan goes to the auction
After the loan is given to new pool, the `loan.auctionlength` is not set according to the new pool.

Even the event to be emitted should include the auction length value.

## Impact
The auction length will be different from the new pool setting.

## Tools Used

## Recommendations
In the function `giveLoan`,`buyLoan` add the following

// update the loan with the new info
+loans[loanId].auctionlength = pools[poolId].auctionLength
loans[loanId].lender = msg.sender;
loans[loanId].interestRate = pools[poolId].interestRate;
loans[loanId].startTimestamp = block.timestamp;
loans[loanId].auctionStartTimestamp = type(uint256).max;
loans[loanId].debt = totalDebt;

## <a id='H-03'></a>H-03. Incorrect setting of the lender while buying loan

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L518

## Summary
Anyone can call `function buyLoan(uint256 loanId, bytes32 poolId)` and become the lender of the loan.

## Vulnerability Details
`buyLoan()` can be called by anyone for any loan and pool. The function then transfers the loan to that pool after all the checks pass. Finally, it sets the lender of the transferred loan to `msg.sender` [here](https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L518):

// update the loan with the new info
loans[loanId].lender = msg.sender;

Since anyone call this function, lender can be set to an address different from the pool's actual lender. After this, if the loan is seized by calling [`seizeLoan()`](https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L566), loan's lender is transferred the collateral:

IERC20(loan.collateralToken).transfer(
        loan.lender,
        loan.collateral - govFee
);

Since `loan.lender` is set incorrectly, the collateral tokens are effectively stolen.

## Impact
Collateral token can be incorrectly transferred

## Tools Used
Manual Review

## Recommendations
Lender assignment should be based on the actual lender of the pool through `poolId.lender`

-loans[loanId].lender = msg.sender;
+loans[loanId].lender = pools[poolId].lender;

## <a id='H-04'></a>H-04. Protocol doesn't support tokens not compatible with ERC20 standard

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L152

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L159

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L187

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L203

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L317-L329

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L403

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L505

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L563-L565

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L642-L656

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L663-L670

## Summary
If non-compliant ERC20 tokens are added to pools (like USDT), then protocol may not be able to handle the transfers.

## Vulnerability Details
The protocol uses `transfer()` and `transferFrom()` to transfer tokens. However, there are popular tokens like USDT which doesn't return data on transfers. Other tokens may return false on failure.

## Impact
Protocol will revert or do incorrect accounting for these tokens.

## Tools Used
Manual

## Recommendations

Use OpenZeppelin's `SafeERC20` library to handle ERC20 transfer. It handles cases where a token may return false on failure or not return anything at all.

Add the following at the top of the contract:

using SafeERC20 for IERC20;

Replace all instances of calling `transfer` and `transferFrom` on ERC20 token with `safeTransfer` and `safeTransferFrom`.
For example for
https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L159

-IERC20(p.loanToken).transfer(
+IERC20(p.loanToken).safeTransfer(

# Medium Risk Findings

## <a id='M-01'></a>M-01. Frontrun issues in the `giveLoan` and `buyLoan` function

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L417

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L519

## Summary
The new pool lender can frontrun a previous pool lender transaction to charge a higher interest from the borrower than expected.

## Vulnerability Details
The borrower can pay higher interest rate than expected. `giveLoan` and `buyLoan` function transfers a loan to a new pool. If the new pool owner sees these transactions in the mempool, it can set its pools' interest rate to the maximum possible (which is loan's current interest rate, or auction's current interest rate).

The loan’s interest rate is coming from `pool.interestRate` which the new pool lender can increase by calling `setPool` or `updateInterestRate` through frontrunning.

## Impact
Borrowers will pay the interest rate higher than expected for their loans. However, `giveLoan` ensures that the interest rate cannot exceed the current loan's interest rate, and `buyLoan` function ensures that the interest rate cannot exceed the auction's interest rate, the risk is limited.

## Tools Used
Manual review.

## Recommendations
Update `giveLoan` and `buyLoan` function to to include `interestRate` argument for each loan being given or bought. This denotes the expected interest rate of the new pool. Revert if it doesn't match with the current value of the new pools's interest rate.
## <a id='M-02'></a>M-02. Borrows can pay higher interest than expected

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Lender.sol#L256

## Summary
The lender can frontrun a borrow transaction to charge a higher interest than the borrower expected.

## Vulnerability Details
The borrower will have to go through a high interest rate even though they saw a lower interest rate at the time of signing the transaction. The loan parameters are created as:

Loan memory loan = Loan({
                lender: pool.lender,
                borrower: msg.sender,
                loanToken: pool.loanToken,
                collateralToken: pool.collateralToken,
                debt: debt,
                collateral: collateral,
                interestRate: pool.interestRate,
                startTimestamp: block.timestamp,
                auctionStartTimestamp: type(uint256).max,
                auctionLength: pool.auctionLength
});

Here, the loan’s interest rate is coming from `pool.interestRate` which the lender can increase by calling `setPool` or `updateInterestRate` through frontrunning.

## Impact
Borrowers will pay the interest rate higher than expected for their loans.

## Tools Used
Manual review.

## Recommendations
Update `Borrow` struct to include `maxInterestRate` field. This denotes the maximum interest rate the borrower is willing to pay. Now revert in `borrow` function if `borrows[I].maxInterestRate < pool.interestRate`.