Git Product home page Git Product logo

2023-07-beedle's Introduction

Beedle

  • Total Prize Pool: $20,000

    • HM Awards: $18,000
    • LQAG: $2,000
  • Starts: July 24th, 2023

  • Ends August 7th, 2023

  • nSLOC: ~706

  • Complexity: ~381

  • Judging Ends August 14th, 2023

Contest Details

Oracle free peer to peer perpetual lending About Beedle - Twitter

Scope

All contracts in src are in scope.

src/
├── Beedle.sol
├── Fees.sol
├── Lender.sol
├── Staking.sol
├── interfaces
│   ├── IERC20.sol
│   └── ISwapRouter.sol
└── utils
    ├── Errors.sol
    ├── Ownable.sol
    └── Structs.sol

Contract Overview

Lender.sol

Lender is the main singleton contract for Beedle. It handles all borrowing, repayment, and refinancing.

Lending

In order to be a lender you must create a lending pool. Lending pools are based on token pairs and any lender can have one pool per lending pair. When creating a pool, lenders choose several key inputs

loanToken - the token they are lending out collateralToken - the token they are taking in as collateral minLoanSize - the minimum loan size they are willing to take (this is to prevent griefing a lender with dust loans) poolBalance - the amount of loanToken they want to deposit into the pool maxLoanRatio - the highest LTV they are willing to take on the loan (this is multiplied by 10^18) auctionLength - the length of liquidation auctions for their loans interestRate - the interest rate they charge for their loans

After creating a pool, lenders can update these values at any time.

Borrowing

Once lending pools are created by lenders, anyone can borrow from a pool. When borrowing you will choose your loanRatio (aka LTV) and the amount you want to borrow. Most other loan parameters are set by the pool creator and not the borrower. After borrowing from a pool there are several things that can happen which we will break down next.

  1. Repaying Repayment is the most simple of all outcomes. When a user repays their loan, they send back the principal and any interest accrued. The repayment goes back to the lenders pool and the user gets their collateral back.
  2. Refinancing Refinancing can only be called by the borrower. In a refinance, the borrower is able to move their loan to a new pool under new lending conditions. The contract does not force the borrower to fully repay their debt when moving potisitions. When refinancing, you must maintain the same loan and collateral tokens, but otherwise, all other parameters are able to be changed.
  3. Giving A Loan When a lender no longer desires to be lending anymore they have two options. They can either send the loan into a liquidation auction (which we will get into next) or they can give the loan to another lender. Lenders can give away their loan at any point so long as, the pool they are giving it to offers same or better lending terms.
  4. Auctioning A Loan When a lender no longer wants to be in a loan, but there is no lending pool available to give the loan to, lenders are able to put the loan up for auction. This is a Dutch Auction where the variable changing over time is the interest rate and it is increasing linearly. Anyone is able to match an active pool with a live auction when the parameters of that pool match that of the auction or are more favorable to the borrower. This is called buying the loan. If the auction finishes without anyone buying the loan, the loan is liquidated. Then the lender is able to withdraw the borrowers collateral and the loan is closed.

Staking.sol

This is a contract based on the code of yveCRV originally created by Andre Cronje. It tracks user balances over time and updates their share of a distribution on deposits and withdraws.

Getting Started

Before diving into the codebase and this implementation of the Blend lending protocol, it is recommended that you read the original paper by Paradigm and Blur

build

forge init

forge install OpenZeppelin/openzeppelin-contracts

forge install vectorized/solady

forge build

test

forge test

deploy

first copy the .example.env file and create your own .env file

forge script script/LenderScript.s.sol:LenderScript --rpc-url $GOERLI_RPC_URL --broadcast --verify -vvvv

Known Issues

No known issues reported.

2023-07-beedle's People

Contributors

0xtingle avatar equious avatar patrickalphac avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar

2023-07-beedle's Issues

`refinance` reduces the pool's balance by the debt twice

refinance reduces the pool's balance by the debt twice

Severity

High Risk

Relevant GitHub Links

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

Summary

refinance accidentally reduces the new pool balance 2 times instead of one, charging it 2x the debt balance.

Vulnerability Details

refinance as the name suggest refinances a loan, this means it can change a few of it's parameters, like reducing or increasing debt or changing the pool it uses and so on.

The current issue with this function is that it accidentally reduces the balances of the new pool twice once here and twice here. This means that it charges the pool of the lender twice instead of once thus lowering his potential profits, messing up the accounting and lowering the amount he can withdraw from the system.

This can be exploited accidentally when a normal user tries to refinance his loan or maliciously when someone want to cause harm on a given user.

Example:
This is Alice's pool

Preconditions values
Loan : Debt tokens USDC : DAI
Max borrow percentage 60%
Pool balance 1000 USDC
  1. Bob sees this pool, but he does not like Alice (for some reason), so he makes his own pool with the same parameter.
  2. Then Bob takes a loan of 300 USDC and provides 500 DAI.
  3. Bob calls refinance on his loan to move it from his pool to Alice's one.
  4. Now because of how refinance works Bob's pool is 100% sloven so he can withdraw everything.

Lets see how the refinance TX went:
Firstly everything is extracted and then checked, as expected the checks pass (because Bob created the exact same pool as Alice's one). Afterwards Bob pays small fees for his action and his pool becomes solvent again. Now Alice's pool gets charged the new debt

            _updatePoolBalance(poolId, pools[poolId].poolBalance - debt);
            pools[poolId].outstandingLoans += debt;

After some transfers to make sure everything is up and right the loan is moved to the new pool where the new pool balances are reduced again.

pools[poolId].poolBalance -= debt;
  1. Now because of the double debt removal, Alice's pool recorded balance (pools[poolId].poolBalance) is 400 USDC (600 less), and her collateral is only 500 DAI

Tools Used

Manual review

Recommendations

Deduct the debt from the pool balance only once, and remove the second reduction.

            loans[loanId].collateral = collateral;
            loans[loanId].interestRate = pool.interestRate;
            loans[loanId].startTimestamp = block.timestamp;
            loans[loanId].auctionStartTimestamp = type(uint256).max;
            loans[loanId].auctionLength = pool.auctionLength;
            loans[loanId].lender = pool.lender;
-           pools[poolId].poolBalance -= debt; //@audit this is not needed

The Function _calculateInterest is not working as intended which results in loss of fee and profit to the protocol

The Function _calculateInterest is not working as intended which results in loss of fee and profit to the protocol

Severity

High Risk

Relevant GitHub Links

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

Summary

The value of interest is not properly calculated which make the whole _calculateInterest function to not perform as intended and loss of fee or profit to the protocol.

Vulnerability Details

The lender.sol consists of the following code:
function _calculateInterest(
Loan memory l
) internal view returns (uint256 interest, uint256 fees) {
uint256 timeElapsed = block.timestamp - l.startTimestamp;
interest = (l.interestRate * l.debt * timeElapsed) / 10000 / 365 days;
fees = (lenderFee * interest) / 10000;
interest -= fees;
}

in which the interest is calculated but the formula of calculating this is the actual problem. As the value is calculated on the basis of this formula[ (l.interestRate * l.debt * timeElapsed) / 10000 / 365 ] days and in this it is stated as per the solidity that the outcome value of (l.interestRate * l.debt * timeElapsed) is to be divide by 10000 which is wrong.
Also the value of (l.interestRate * l.debt * timeElapsed) is first divided by 10000 and then by 365 which makes the interest value wrong.

Example :
If the value (l.interestRate * l.debt * timeElapsed) results in 30000 and according to formula it is first divided by 10000 then the value 3 is left and then it divides by 365 which is in decimal and very wrong(small).

To behave intended and right the value (l.interestRate * l.debt * timeElapsed) must be divide by the result of (10000/365) which gives the proper value of interest and after that fees for the protocol.

Impact

Impact of it is that if the value of interest is calculated as 0.0something which results in the very low value of the fees to the protocol as in the fees formula the division value is very large. Which results in loss of fees or profit to your protocol.

Tools Used

Manual

Recommendations

Do changes to the interest formula like this.
interest = (l.interestRate * l.debt * timeElapsed) / (10000 / 365 days);
by this the whole value is calculated by the result of (10000 / 365 days) and gives the right interest an right fees.

Single-Step Ownership transfer

Single-Step Ownership transfer

Severity

Medium Risk

Summary

Single-step process for critical ownership transfer

Vulnerability Details

The current ownership transfer process involves the current owner calling transferOwnership(). This function have one step ownership transfer and it doesn't check that the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.

Issue Location

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Ownable.sol#L19-L22

Impact

If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various deployer contract addresses and market approvals. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of the factory contract and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in markets and incur a significant reputational damage.

Tools Used

Manual Review

Recommendations

use a two-step ownership transfer pattern and add a zero-address check.

More known issues

Report

Gas Optimizations

Issue Instances
GAS-1 Cache array length outside of loop 6
GAS-2 Use Custom Errors 2
GAS-3 Don't initialize variables with default value 8
GAS-4 Functions guaranteed to revert when called by normal users can be marked payable 5
GAS-5 ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) 6
GAS-6 Using private rather than public for constants, saves gas 3
GAS-7 Use != 0 instead of > 0 for unsigned integer comparison 5

[GAS-1] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (6):

File: src//Lender.sol

233:         for (uint256 i = 0; i < borrows.length; i++) {

293:         for (uint256 i = 0; i < loanIds.length; i++) {

359:         for (uint256 i = 0; i < loanIds.length; i++) {

438:         for (uint256 i = 0; i < loanIds.length; i++) {

549:         for (uint256 i = 0; i < loanIds.length; i++) {

592:         for (uint256 i = 0; i < refinances.length; i++) {

[GAS-2] Use Custom Errors

Source
Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.

Instances (2):

File: src//Fees.sol

27:         require(_profits != WETH, "not allowed");
File: src//utils/Ownable.sol

11:         require(msg.sender == owner, "UNAUTHORIZED");

[GAS-3] Don't initialize variables with default value

Instances (8):

File: src//Lender.sol

233:         for (uint256 i = 0; i < borrows.length; i++) {

293:         for (uint256 i = 0; i < loanIds.length; i++) {

359:         for (uint256 i = 0; i < loanIds.length; i++) {

438:         for (uint256 i = 0; i < loanIds.length; i++) {

549:         for (uint256 i = 0; i < loanIds.length; i++) {

592:         for (uint256 i = 0; i < refinances.length; i++) {
File: src//Staking.sol

14:     uint256 public balance = 0;

16:     uint256 public index = 0;

[GAS-4] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Instances (5):

File: src//Beedle.sol

36:     function mint(address to, uint256 amount) external onlyOwner {
File: src//Lender.sol

84:     function setLenderFee(uint256 _fee) external onlyOwner {

92:     function setBorrowerFee(uint256 _fee) external onlyOwner {

100:     function setFeeReceiver(address _feeReceiver) external onlyOwner {
File: src//utils/Ownable.sol

19:     function transferOwnership(address _owner) public virtual onlyOwner {

[GAS-5] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (6):

File: src//Lender.sol

233:         for (uint256 i = 0; i < borrows.length; i++) {

293:         for (uint256 i = 0; i < loanIds.length; i++) {

359:         for (uint256 i = 0; i < loanIds.length; i++) {

438:         for (uint256 i = 0; i < loanIds.length; i++) {

549:         for (uint256 i = 0; i < loanIds.length; i++) {

592:         for (uint256 i = 0; i < refinances.length; i++) {

[GAS-6] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Instances (3):

File: src//Fees.sol

16:     ISwapRouter public constant swapRouter =
File: src//Lender.sol

59:     uint256 public constant MAX_INTEREST_RATE = 100000;

61:     uint256 public constant MAX_AUCTION_LENGTH = 3 days;

[GAS-7] Use != 0 instead of > 0 for unsigned integer comparison

Instances (5):

File: src//Staking.sol

63:         if (totalSupply > 0) {

67:                 if (_diff > 0) {

69:                     if (_ratio > 0) {

83:         if (_supplied > 0) {

87:             if (_delta > 0) {

Non Critical Issues

Issue Instances
NC-1 Typos 23

[NC-1] Typos

Instances (23):

File: src//Fees.sol

- 24:     /// @notice swap loan tokens for collateral tokens from liquidations
+ 24:     /// @notice swap collateral tokens for loan tokens from liquidations

- 25:     /// @param _profits the token to swap for WETH
+ 25:     /// @param _profits the token to swap for ETH
File: src//Lender.sol

- 62:     /// @notice the fee taken by the protocol in BIPs
+ 62:     /// @notice the fee taken by the protocol in BPS

- 69:     /// @notice mapping of poolId to Pool (poolId is keccak256(lender, loanToken, collateralToken))
+ 69:     /// @notice mapping of poolId to Pool (poolId is keccak256(lender, loanToken, collateralToken)

- 545:     /// @notice sieze a loan after a failed refinance auction
+ 545:     /// @notice seize a loan after a failed refinance auction

- 547:     /// @param loanIds the ids of the loans to sieze
+ 547:     /// @param loanIds the ids of the loans to seize

- 635:             // now lets deduct our tokens from the new pool
+ 635:             // now let's deduct our tokens from the new pool
File: src//Staking.sol

- 13:     /// @notice the balance of reward tokens
+ 13:     /// @notice The balance of reward tokens

- 15:     /// @notice the index of the last update
+ 15:     /// @notice The index of the last update

- 18:     /// @notice mapping of user indexes
+ 18:     /// @notice Mapping of user indexes

- 21:     /// @notice mapping of user balances
+ 21:     /// @notice Mapping of user balances

- 23:     /// @notice mapping of user claimable rewards
+ 23:     /// @notice Mapping of user claimable rewards

- 26:     /// @notice the staking token
+ 26:     /// @notice The staking token

- 28:     /// @notice the reward token
+ 28:     /// @notice The reward token

- 36:     /// @notice deposit tokens to stake
+ 36:     /// @notice Deposit tokens to stake

- 37:     /// @param _amount the amount to deposit
+ 37:     /// @param _amount The amount to deposit

- 44:     /// @notice withdraw tokens from stake
+ 44:     /// @notice Withdraw tokens from stake

- 45:     /// @param _amount the amount to withdraw
+ 45:     /// @param _amount The amount to withdraw

- 52:     /// @notice claim rewards
+ 52:     /// @notice Claim rewards

- 60:     /// @notice update the global index of earned rewards
+ 60:     /// @notice Update the global index of earned rewards

- 78:     /// @notice update the index for a user
+ 78:     /// @notice Update the index for a user

- 79:     /// @param recipient the user to update
+ 79:     /// @param recipient The user to update
File: src//utils/Structs.sol

- 75:     /// @notice the virtual balance based on the multipier
+ 75:     /// @notice the virtual balance based on the multiplier

Low Issues

Issue Instances
L-1 Unsafe ERC20 operation(s) 24

[L-1] Unsafe ERC20 operation(s)

Instances (24):

File: src//Fees.sol

43:         IERC20(WETH).transfer(staking, IERC20(WETH).balanceOf(address(this)));
File: src//Lender.sol

152:             IERC20(p.loanToken).transferFrom(

159:             IERC20(p.loanToken).transfer(

187:         IERC20(pools[poolId].loanToken).transferFrom(

203:         IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);

267:             IERC20(loan.loanToken).transfer(feeReceiver, fees);

269:             IERC20(loan.loanToken).transfer(msg.sender, debt - fees);

271:             IERC20(loan.collateralToken).transferFrom(

317:             IERC20(loan.loanToken).transferFrom(

323:             IERC20(loan.loanToken).transferFrom(

329:             IERC20(loan.collateralToken).transfer(

403:             IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);

505:         IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);

563:             IERC20(loan.collateralToken).transfer(feeReceiver, govFee);

565:             IERC20(loan.collateralToken).transfer(

642:                 IERC20(loan.loanToken).transferFrom(

651:                 IERC20(loan.loanToken).transfer(feeReceiver, fee);

653:                 IERC20(loan.loanToken).transfer(msg.sender, debt - debtToPay - fee);

656:             IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);

663:                 IERC20(loan.collateralToken).transferFrom(

670:                 IERC20(loan.collateralToken).transfer(
File: src//Staking.sol

39:         TKN.transferFrom(msg.sender, address(this), _amount);

49:         TKN.transfer(msg.sender, _amount);

55:         WETH.transfer(msg.sender, claimable[msg.sender]);

Medium Issues

Issue Instances
M-1 Centralization Risk for trusted owners 12

[M-1] Centralization Risk for trusted owners

Impact:

Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.

Instances (12):

File: src//Beedle.sol

9: contract Beedle is Ownable, ERC20, ERC20Permit, ERC20Votes {

11:     constructor() ERC20("Beedle", "BDL") ERC20Permit("Beedle") Ownable(msg.sender) {

36:     function mint(address to, uint256 amount) external onlyOwner {
File: src//Lender.sol

10: contract Lender is Ownable {

73:     constructor() Ownable(msg.sender) {

84:     function setLenderFee(uint256 _fee) external onlyOwner {

92:     function setBorrowerFee(uint256 _fee) external onlyOwner {

100:     function setFeeReceiver(address _feeReceiver) external onlyOwner {
File: src//Staking.sol

11: contract Staking is Ownable {

31:     constructor(address _token, address _weth) Ownable(msg.sender) {
File: src//utils/Ownable.sol

4: abstract contract Ownable {

19:     function transferOwnership(address _owner) public virtual onlyOwner {

Deposit function can lead to a potential reentrancy vulnerability.

Deposit function can lead to a potential reentrancy vulnerability.

Severity

Medium Risk

Relevant GitHub Links

function deposit(uint _amount) external {
TKN.transferFrom(msg.sender, address(this), _amount);
updateFor(msg.sender);
balances[msg.sender] += _amount;
}

Vulnerability Details
The function deposit allows a user to transfer tokens (_amount) from their address to the contract (address(this)) using the transferFrom function. The transferFrom function can trigger an external contract call. If the external contract's code is malicious and designed to execute a reentrancy attack, it can potentially call back into the deposit function before it completes.

Affectd Code:

function deposit(uint _amount) external {
TKN.transferFrom(msg.sender, address(this), _amount);
updateFor(msg.sender);
balances[msg.sender] += _amount;
}

Impact

If the external contract's code is malicious and designed to execute a reentrancy attack, it can potentially call back into the deposit function before it completes.

Tools Used

Manual

Recommendations

To protect against reentrancy, you should follow the "checks-effects-interactions" pattern and ensure that you perform all necessary checks and updates before making any external calls. To do this, you should first update the user's balance and then transfer the tokens.

Suggested Code:

 function deposit(uint _amount) external {
    updateFor(msg.sender);
    balances[msg.sender] += _amount;
    TKN.transferFrom(msg.sender, address(this), _amount);
}

Increments can be unchecked

Increments can be unchecked

Severity

Gas Optimization

Relevant GitHub Links

ethereum/solidity#10695

Summary

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Vulnerability Details

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

Instances included:

Lender.borrow() #232      for (uint256 i = 0; i < borrows.length; i++) {

Lender.repay() #292      for (uint256 i = 0; i < loanIds.length; i++) {

Lender.giveLoan() #355      for (uint256 i = 0; i < loanIds.length; i++) {

Lender.startAuction() #437      for (uint256 i = 0; i < loanIds.length; i++) {

Lender.seizeLoan() #548      for (uint256 i = 0; i < loanIds.length; i++) {

Lender.refinance() #591      for (uint256 i = 0; i < refinances.length; i++) {

Tools Used

Manual

Recommendations

The code would go from:

for (uint256 i = 0; i < [].length; i++) {  
 // ...  
}  

to

for (uint256 i = 0; i < [].length;) {  
 // ...  
 unchecked { i++; }  
}  

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Severity

Low Risk

Relevant GitHub Links

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

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

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

Summary

Vulnerability Details

While the compiler knows to optimize away the exponentiation, it’s still better coding practice to use idioms that do not require compiler optimization, if they exist.

Impact

Tools Used

Recommendations

Fee on transfer or deflationary erc20 tokens not taken into account while calculating poolBalance which can lead to loss of funds.

Fee on transfer or deflationary erc20 tokens not taken into account while calculating poolBalance which can lead to loss of funds.

Severity

Medium Risk

Relevant GitHub Links

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

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

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

Summary

The balance of the pool (pool.poolBalance) is set without accounting for 'fee on transfer' or deflationary tokens which can lead to wrong balance values causing unintended issues as it is used by many functions for calculations.

Vulnerability Details

setPool(Pool calldata p) -> this function sets the pool (pools[poolId] = p;).
If the erc20 token has a 'fee on transfer' mechanism then the poolBalance will be set wrongly as they are not accounted for.

Same issue in the
addToPool(bytes32 poolId, uint256 amount) function -> this function updates the pool balance using.
_updatePoolBalance(poolId, pools[poolId].poolBalance + amount). which can go wrong with fee on transfer tokens.

Impact

Users can take more debt than pool Balance, which will lead to loss of funds.
borrow(Borrow[] calldata borrows) -> this function checks for balance before lending (if (debt > pool.poolBalance) revert LoanTooLarge();). Since the pool.poolBalance can be larger than the actual balance of erc20 in the pool of the contract, users can take more debt than intended which can drain contract's balance of the token.

Tools Used

Manual review

Recommendations

Either White list specific erc20 tokens for use in the protocol or account for fee on transfer and deflationary tokens during calculation of pool.poolBalance

Staking deposit function transferFrom first and then updateFor causing partial reward get stuck in the contract forever

Staking deposit function transferFrom first and then updateFor causing partial reward get stuck in the contract forever

Severity

High Risk

Relevant GitHub Links

TKN.transferFrom(msg.sender, address(this), _amount);
updateFor(msg.sender);

Summary

Staking deposit function transferFrom first and then updateFor, transferFrom will increase totalSupply and decrease the reward index, which resulted in some of the rewards never being claimed, stuck in the contract.

Vulnerability Details

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "solady/src/tokens/ERC20.sol";
import "../src/Staking.sol";

contract SERC20 is ERC20 {
    function name() public pure override returns (string memory) {
        return "Test ERC20";
    }

    function symbol() public pure override returns (string memory) {
        return "TERC20";
    }

    function mint(address _to, uint256 _amount) public {
        _mint(_to, _amount);
    }
}

contract StakingTest is Test {
    SERC20 st;
    SERC20 weth;
    Staking staking;

    function setUp() public {
        st = new SERC20();
        weth = new SERC20();
        staking  = new Staking(address(st), address(weth));
    }
    
    function testDeposit() public {
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");

        deal(address(weth), address(staking), weth.balanceOf(address(staking)) + 1 ether);
        deal(address(st), address(alice), 2 ether);
        deal(address(st), address(bob), 2 ether);

        vm.startPrank(bob);
        st.approve(address(staking), 2 ether);
        staking.deposit(1 ether);
        staking.claim();
        vm.stopPrank();
        // @audit Bob won't be able to claim any rewards, which are stuck in the contract
        assertEq(weth.balanceOf(bob), 0);

        vm.roll(100);
        deal(address(weth), address(staking), weth.balanceOf(address(staking)) + 1 ether);

        vm.startPrank(alice);
        st.approve(address(staking), 2 ether);
        staking.deposit(1 ether);
        staking.claim();
        vm.stopPrank();
        vm.startPrank(bob);
        staking.claim();
        vm.stopPrank();
        // @audit Alice won't be able to claim any rewards
        assertEq(weth.balanceOf(alice), 0);
        // @audit Bob can only gets half the reward
        assertEq(weth.balanceOf(bob), 0.5 ether);

        vm.roll(200);

        vm.startPrank(alice);
        staking.claim();
        vm.stopPrank();
        vm.startPrank(bob);
        staking.claim();
        vm.stopPrank();
        // @audit The rest of the reward cannot be claimed and is locked in the contract forever
        assertEq(weth.balanceOf(alice), 0);
        assertEq(weth.balanceOf(bob), 0.5 ether);

        vm.roll(300);
        deal(address(weth), address(staking), weth.balanceOf(address(staking)) + 1 ether);

        vm.startPrank(alice);
        staking.deposit(1 ether);
        staking.claim();
        vm.stopPrank();
        vm.startPrank(bob);
        staking.claim();
        vm.stopPrank();
        // @audit Alice and Bob can only get 1/3 reward because of totalSupply
        uint256 reward = 1 ether;
        reward /= 3;
        assertEq(weth.balanceOf(alice), reward);
        assertEq(weth.balanceOf(bob), 0.5 ether + reward);
    }
}

The above contract shows the effect of a change in totalSupply resulting from executing transferFrom first on the distribution of rewards. There are three main scenarios:

  1. When there is no staking user, the transferred rewards are permanently stuck in the contract
  2. If a staking user not claiming, then another user deposits with a totalSupply change, the staking user's reward will be halved and the remaining stuck in the contract
  3. If both users are staking the same amount and deposit the same amount again, each user gets 1/3 and the remaining stuck in the contract

Impact

Staking deposit function transferFrom first and then updateFor may causing partial reward get stuck in the contract forever.

Tools Used

Foundry

Recommendations

Call updateFor first and then transferFrom

Check-Effect-Interaction is not enforced

Check-Effect-Interaction is not enforced

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Staking.sol#L53C1-L58C6

Summary

The claim function of the staking contract is setting the claimable mapping after a token transfer.

Solidity recommends the usage of the Check-Effects-Interaction Pattern to avoid potential security issues, such as reentrancy.

Vulnerability Details

claim function of Staking.sol contract is updating the claimable mapping after the external call which can be used by attacker to control the flow of the contract and further reenter into the function. It is advisable to follow CEI pattern.

Impact

May cause minimal or un-noticeable impact, But this should be remediated as external call may lead of reentrancy to other functions as well.

Tools Used

manual

Recommendations

Although the impact is very limited, it is recommended to implement:

  1. modifier such as nonReentrant

  2. checks-effects-interactions pattern.

Return values of `transfer()`/`transferFrom()` not checked

Return values of transfer()/transferFrom() not checked

Severity

Medium Risk

Relevant GitHub Links

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Summary

The return values of transfer()/transferFrom() are unchecked.

Vulnerability Details:

Not all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

Code Snippet

File: Fees.sol

43:         IERC20(WETH).transfer(staking, IERC20(WETH).balanceOf(address(this)));
File: Lender.sol

152:         IERC20(p.loanToken).transferFrom(
153:             p.lender,
154:             address(this),
155:             p.poolBalance - currentBalance
156:        );


159:         IERC20(p.loanToken).transfer(
160:             p.lender,
161:             currentBalance - p.poolBalance
162:        );


187:        IERC20(pools[poolId].loanToken).transferFrom(
188:            msg.sender,
189:            address(this),
190:            amount
191:        );


203:        IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);


267:        IERC20(loan.loanToken).transfer(feeReceiver, fees);
268:        // transfer the loan tokens from the pool to the borrower
269:        IERC20(loan.loanToken).transfer(msg.sender, debt - fees);
270:        // transfer the collateral tokens from the borrower to the contract
271:        IERC20(loan.collateralToken).transferFrom(
272:            msg.sender,
273:            address(this),
274:            collateral
275:        );


317:        IERC20(loan.loanToken).transferFrom(
318:            msg.sender,
319:            address(this),
320:            loan.debt + lenderInterest
321:        );
322:        // transfer the protocol fee to the fee receiver
323:        IERC20(loan.loanToken).transferFrom(
324:            msg.sender,
325:            feeReceiver,
326:            protocolInterest
327:        );
328:        // transfer the collateral tokens from the contract to the borrower
329:        IERC20(loan.collateralToken).transfer(
330:            loan.borrower,
331:            loan.collateral
322:        );


403:        IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);


505:        IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);


563:        IERC20(loan.collateralToken).transfer(feeReceiver, govFee);
564:        // transfer the collateral tokens from the contract to the lender
565:        IERC20(loan.collateralToken).transfer(
566:            loan.lender,
567:            loan.collateral - govFee
568:        );


642:        IERC20(loan.loanToken).transferFrom(
643:            msg.sender,
644:            address(this),
645:            debtToPay - debt
646:        );

663:         IERC20(loan.collateralToken).transferFrom(
664:             msg.sender,
665:             address(this),
666:             collateral - loan.collateral
667:         );

670:         IERC20(loan.collateralToken).transfer(
671:             msg.sender,
672:             loan.collateral - collateral
673:         );
File: Staking.sol

39:         TKN.transferFrom(msg.sender, address(this), _amount);

49:         TKN.transfer(msg.sender, _amount);

55:         WETH.transfer(msg.sender, claimable[msg.sender]);

Impact

May potentially proceed without making any payment.

Tools Used

Manual

Recommendations

Return values of transfer()/transferFrom() should be checked.

Staking contracts should be assert TKN != WETH

Staking contracts should be assert TKN != WETH

Severity

Low Risk

Relevant GitHub Links

function withdraw(uint _amount) external {
updateFor(msg.sender);
balances[msg.sender] -= _amount;
TKN.transfer(msg.sender, _amount);
}

Summary

The staking contract is incompatible with the same address of TKN and WETH. So you should force them to be different address in the constructor, otherwise it will mess up the internal accounting system.

Vulnerability Details

    function claim() external {
        updateFor(msg.sender);
        WETH.transfer(msg.sender, claimable[msg.sender]);
        claimable[msg.sender] = 0;
        balance = WETH.balanceOf(address(this));
    }

    function update() public {
        uint256 totalSupply = TKN.balanceOf(address(this));
        if (totalSupply > 0) {
            uint256 _balance = WETH.balanceOf(address(this));
            if (_balance > balance) {
                uint256 _diff = _balance - balance;
                if (_diff > 0) {
                    uint256 _ratio = _diff * 1e18 / totalSupply;
                    if (_ratio > 0) {
                      index = index + _ratio;
                      balance = _balance;
                    }
                }
            }
        }
    }

The contract is not compatible with this situation such as the balance update in the claim function, totalSupply confuses the collateral and reward, which will disrupte internal accounting.

Impact

Staking contracts is not compatible with the same TKN and WETH, which will disrupte internal accounting.

Tools Used

Manual review

Recommendations

Assert TKN != WETH

caching the array length into a memory variable in loops

caching the array length into a memory variable in loops

Severity

Gas Optimization

Relevant GitHub Links

for (uint256 i = 0; i < borrows.length; i++) {

for (uint256 i = 0; i < loanIds.length; i++) {

for (uint256 i = 0; i < loanIds.length; i++) {

for (uint256 i = 0; i < loanIds.length; i++) {

for (uint256 i = 0; i < loanIds.length; i++) {

for (uint256 i = 0; i < refinances.length; i++) {

Summary

storing the length of the array in a variable is more gas efficient

Vulnerability Details

storing the length of the array in a variable is more gas efficient and save one calldatacopy (3 gas )

Impact

gas saved 6 * 3 = 18 gas unit

Tools Used

manual review

Recommendations

cache the length of the array in local variable before starting the loop

Unsafe use of `transfer()`/`transferFrom()` with `IERC20`

Unsafe use of transfer()/transferFrom() with IERC20

Severity

Medium Risk

Relevant GitHub Links

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

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

Summary

Unsafe use of transfer()/transferFrom() with IERC20

Vulnerability Details

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert (see this link for a test case).

Code Snippet

File: Lender.sol

152:            IERC20(p.loanToken).transferFrom(
153:                p.lender,
154:                address(this),
155:                p.poolBalance - currentBalance
156:            );
157:        } else if (p.poolBalance < currentBalance) {
158:            // if new balance < current balance then transfer the difference back to the lender
159:            IERC20(p.loanToken).transfer(
160:                p.lender,
161:                currentBalance - p.poolBalance
162:            );
163:        }
File: Lender.sol

267:            IERC20(loan.loanToken).transfer(feeReceiver, fees);
268:            // transfer the loan tokens from the pool to the borrower
269:            IERC20(loan.loanToken).transfer(msg.sender, debt - fees);
270:            // transfer the collateral tokens from the borrower to the contract
271:            IERC20(loan.collateralToken).transferFrom(
272:                msg.sender,
273:                address(this),
274:                collateral
275:           );

Impact

Tools Used

Manual

Recommendations

Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

caching variable of struct in one slot

caching variable of struct in one slot

Severity

Gas Optimization

Relevant GitHub Links

struct Pool {
/// @notice address of the lender
address lender;
/// @notice address of the loan token
address loanToken;
/// @notice address of the collateral token
address collateralToken;
/// @notice the minimum size of the loan (to prevent griefing)
uint256 minLoanSize;
/// @notice the maximum size of the loan (also equal to the balance of the lender)
uint256 poolBalance;
/// @notice the max ratio of loanToken/collateralToken (multiplied by 10**18)
uint256 maxLoanRatio;
/// @notice the length of a refinance auction
uint256 auctionLength;
/// @notice the interest rate per year in BIPs
uint256 interestRate;
/// @notice the outstanding loans this pool has
uint256 outstandingLoans;
}

Summary

store more than one variable in one storage slot save gas

Vulnerability Details

in struct Pool there are 3 variable can be packed into one storage slot .
// uint96 maxLoanRatio has a max value of 118 so uint96 is more efficient
// uint80 auctionLength has a max value of 3 days and this storage space can store 3
16 years
// uint80 interest rate has a max value of 1000*100

Impact

gas saving
Each slot saved can avoid an extra Gas (20000 gas) for the first setting of the struct
store the variable in one storage slot is more gas efficient .

Tools Used

manual review

Recommendations

using
// uint64 maxLoanRatio
// uint24 auctionLength
// uint24 interest rate
instead of
// uint256 maxLoanRatio
// uint256 auctionLength
// uint256 interest rate

<array>.length should not be looked up in every loop of a for-loop

.length should not be looked up in every loop of a for-loop

Severity

Gas Optimization

Relevant GitHub Links

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

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

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

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

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

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

Vulnerability Details

The overheads outlined below are PER LOOP, excluding the first loop

  • storage arrays incur a Gwarmaccess (100 gas)
  • memory arrays use MLOAD (3 gas)
  • calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

Using `private` rather than `public` for constants, saves gas

Using private rather than public for constants, saves gas

Severity

Gas Optimization

Relevant GitHub Links

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

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

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

Summary

Saves deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

Vulnerability Details

Impact

Gas Consumption

Tools Used

Recommendations

Reentrancy in Claim() at Staking.sol

Reentrancy in Claim() at Staking.sol

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Staking.sol#L53-58

Vulnerability Details

It was found that claim() function of Staking.sol doesn't implement checks-effects-interactions pattern therefore leaving the function vulnerable to reentrancy attacks.

    function claim() external {
        updateFor(msg.sender);
        WETH.transfer(msg.sender, claimable[msg.sender]);
        claimable[msg.sender] = 0;
        balance = WETH.balanceOf(address(this));
    }

the function transfers the claimable balance to the msg.sender first and then in the next line updates the state with claimable[msg.sender] = 0;

Impact

An attacker could exploit the contract to re-enter the claim() function several times leading to drain it from funds.

Tools Used

Manual Review.

Recommendations

It is advisable to use checks-effects-interactions pattern and to use nonReentrant() modifier as an added layer of security.

A slippage of 0 allows anyone to steal the fee contract funds

A slippage of 0 allows anyone to steal the fee contract funds

Severity

High Risk

Relevant GitHub Links

amountOutMinimum: 0,

Summary

In the Fee contract, the funds in the contract will be swapped to WETH, and then will be deposited in the Staking contract as incentives.
But since the slippage is 0 during the swap, anyone can steal all the funds through a sandwich or JIT attack, and no WETH will be credited as a reward to the staking contract.

Vulnerability Details

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _profits,
                tokenOut: WETH,
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: amount,
                amountOutMinimum: 0,
                sqrtPriceLimitX96: 0
            });
  1. Anyone can call sellProfits so they don't need to listen to the mempool but trigger it themselves
  2. The malicious users push the tick in the uniswap3 pool to a high level, and then swap through a sandwich or JIT, the minimum WETH received can be 0, and then restore the tick in the pool to the market price, the attacker can steal almost all the tokens of the swap.

Impact

A malicious user can steal all the tokens in the Fee contract, the staking contract will not be incentivized, and no users will want to staking.

Tools Used

Manual review

Recommendations

Fix a percentage slippage or set the owner to manage the slippage.

using x=x+y is more gas efficient than x+=y

using x=x+y is more gas efficient than x+=y

Severity

Gas Optimization / Informational

Relevant GitHub Links

pools[poolId].outstandingLoans += debt;

pools[poolId].outstandingLoans += totalDebt;

pools[poolId].outstandingLoans += totalDebt;

pools[poolId].outstandingLoans += debt;

pools[oldPoolId].outstandingLoans -= loan.debt;

pools[poolId].outstandingLoans -= loan.debt;

pools[oldPoolId].outstandingLoans -= loan.debt;

pools[poolId].outstandingLoans -= loan.debt;

pools[oldPoolId].outstandingLoans -= loan.debt;

pools[poolId].poolBalance -= debt;

Summary

using x=x+y is more gas efficient than x+=y

Vulnerability Details

x += y/x -= y costs more gas than x = x + y/x = x - y for state variables .

Impact

save 20 gas unit per instance
gas saved = 20 * 10 = 200

Tools Used

manual review

Recommendations

use x=x+y instead of x+=y

No slippage check in `sellProfits`, it could be exploited by a MEV bot

No slippage check in sellProfits, it could be exploited by a MEV bot

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Fees.sol#L30-L40

Summary

The code sets amountOutMinimum to 0 in ExactInputSingleParams, potentially enabling exploitation by MEVs.

Vulnerability Details

From code below, the variable amountOutMinimum is assigned a value of 0. In the context of ExactInputSingleParams, it is understood that amountOutMinimum represents the minimum expected output. If the actual output falls below this specified minimum, the UNI contract will revert the transaction. However, setting amountOutMinimum to 0 can potentially lead to exploitation, as it allows for significant slippage in the token's value before executing the transaction. Consequently, transactions like these become attractive targets for MEVs.

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

Impact

TX could be gamed for max slippage extraction.

Tools Used

Manual Review

Recommendations

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

Missing zero check in Ownable in constructor and transferOwnership

Missing zero check in Ownable in constructor and transferOwnership

Severity

Low Risk

Relevant GitHub Links

constructor(address _owner) {
owner = _owner;
emit OwnershipTransferred(address(0), _owner);
}
function transferOwnership(address _owner) public virtual onlyOwner {
owner = _owner;
emit OwnershipTransferred(msg.sender, _owner);
}
}

Summary

In functions constructor and transferOwnership missing zero address check. If by mistake admin call transferOwnership with zero address protocol will be continue without owner and this can cause errors.

Vulnerability Details

missing zero check for _owner address

Impact

Can transfer ownership to zero address

Tools Used

Manual Review

Recommendations

in both function add
require(_owner != address(0));

Unlimited slippage in Fees.sellProfits() as amountOutMinimum: 0

Unlimited slippage in Fees.sellProfits() as amountOutMinimum: 0

Severity

High Risk

Relevant GitHub Links

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

Summary

Unlimited slippage in Fees.sellProfits() as amountOutMinimum: 0

Vulnerability Details

Setting amountOutMinimum: 0 means that the caller is happy to accept 0 output tokens for their supplied input tokens; this exposes the swap to unlimited slippage from sandwich attacks.

Impact

Fees.sellProfits() exposed to unlimited slippage, can swap supplied input tokens for 0 output tokens.

Tools Used

Manual

Recommendations

Allow caller to specify slippage parameter. UniswapV3 docs warn about this: "amountOutMinimum: we are setting to zero, but this is a significant risk in production. For a real deployment, this value should be calculated using our SDK or an onchain price oracle - this helps protect against getting an unusually bad price for a trade due to a front running sandwich or another type of price manipulation"

Unchecked ERC20 transfers can lead to funds draining/freezing

Unchecked ERC20 transfers can lead to funds draining/freezing

Severity

High Risk

Relevant GitHub Links

sherlock-audit/2023-01-cooler-judging#335

https://github.com/d-xo/weird-erc20/#no-revert-on-failure

Summary

Some major tokens went live before ERC20 was finalised, resulting in a discrepancy whether the transfer functions a) should return a boolean or b) revert/fail on error. The current best practice is that they should revert, but return “true” on success. However, not every token claiming ERC20-compatibility is doing this — some only return true/false; some revert, but do not return anything on success. This is a well known issue, heavily discussed since mid-2018.

Vulnerability Details

Some tokens do not revert on failure, but instead return false (e.g. ZRX). tranfser/transferfrom is directly used to send tokens in many places in the contract and the return value is not checked.
If the token transfer fails, it will cause a lot of serious problems.

As document stated, the lender can freely specify the pool's loan/collateral tokens, so there are chances that these tokens are used in production.

Some examples of how much damage non-revert tokens on failure can cause in production:

In the addToPool function, if loan token is ZRX, the lender can increase the pool balance without providing any loan token and then drain other lenders' deposits of loan token via removeFromPool function.

_updatePoolBalance(poolId, pools[poolId].poolBalance + amount);
// transfer the loan tokens from the lender to the contract
IERC20(pools[poolId].loanToken).transferFrom(
   msg.sender,
   address(this),
   amount
);

Also this can affect the removeFromPool function, if the transfer of loan token to the lender somehow fails, the lender's loan token is frozen forever in the contract since poolBalance has been decreased by the amount but the token is not transferred successfully to the lender.

 _updatePoolBalance(poolId, pools[poolId].poolBalance - amount);
 // transfer the loan tokens from the contract to the lender
 IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);

For borrower's cases, it can be exploited in the repay function, if loan token is ZRX, the borrower can repay the loan without providing any loan token.

// transfer the loan tokens from the borrower to the pool
IERC20(loan.loanToken).transferFrom(
   msg.sender,
   address(this),
   loan.debt + lenderInterest
);
// transfer the protocol fee to the fee receiver
IERC20(loan.loanToken).transferFrom(
   msg.sender,
   feeReceiver,
   protocolInterest
);

And if the collateral token is ZRX, the borrower can borrow any amount of loan token without providing collateral assets.

IERC20(loan.collateralToken).transferFrom(
   msg.sender,
   address(this),
   collateral
);
loans.push(loan);

Impact

If loan or collateral token used in the pool is not reverting on failures, it can be exploited to drain liquidity in the pool as well as blindly freeze assets of lender forever in the contract.

Tools Used

Manual

Recommendations

  1. Using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Gas stipend for external call might be insufficient and lead to stuck ETH

Gas stipend for external call might be insufficient and lead to stuck ETH

Severity

Medium Risk

Relevant GitHub Links

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

Summary

There is a gas stipend of 3000, but this might not be enough in some cases as some smart contract recipients need more than 3000 gas to receive ETH.

Vulnerability Details

The Fees.sol file contains the following code:

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

    amount = swapRouter.exactInputSingle(params);
    IERC20(WETH).transfer(staking, IERC20(WETH).balanceOf(address(this)));

As you see, there is a gas stipend of 3000, but this might not be enough in some cases as some smart contract recipients need more than 3000 gas to receive ETH.

Examples of problematic recipients:

1.Recipient is a smart contract that has a payable fallback method which uses more than 3000 gas.

2.Recipient is a smart contract that has a payable fallback function that needs less than 3000 gas but is called through a proxy, raising the call's gas usage above 3000.

Additionally, using higher than 3000 gas might be mandatory for some multi-sig wallets.

Impact

Some recipients will lose access to all of their profit ETH from protocols that are integrated with beedle. This requires a special type of recipient, so it is Medium severity.

Tools Used

Manual Test

Recommendations

At least doubling down the gas stipend should help in most scenarios, but maybe think about dynamic configuration options for it as well.

Frontrun can get the full reward, no staking time required

Frontrun can get the full reward, no staking time required

Severity

Medium Risk

Relevant GitHub Links

function update() public {

Summary

Anyone can get all the rewards as long as they call deposit before the reward distribution, without a long time staking, after receiving the reward, the user can withdraw the collateral and conduct other transactions. This would result in no user being willing to keep staking collateral.

Vulnerability Details

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "solady/src/tokens/ERC20.sol";
import "../src/Staking.sol";

contract SERC20 is ERC20 {
    function name() public pure override returns (string memory) {
        return "Test ERC20";
    }

    function symbol() public pure override returns (string memory) {
        return "TERC20";
    }

    function mint(address _to, uint256 _amount) public {
        _mint(_to, _amount);
    }
}

contract StakingTest is Test {
    SERC20 st;
    SERC20 weth;
    Staking staking;

    function setUp() public {
        st = new SERC20();
        weth = new SERC20();
        staking  = new Staking(address(st), address(weth));
    }
    
    function testDeposit() public {
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");

        deal(address(st), address(alice), 2 ether);
        deal(address(st), address(bob), 2 ether);

        vm.startPrank(bob);
        st.approve(address(staking), 2 ether);
        staking.deposit(2 ether);
        vm.stopPrank();

        vm.roll(100);

        vm.startPrank(alice);
        st.approve(address(staking), 2 ether);
        staking.deposit(2 ether);
        vm.stopPrank();

        deal(address(weth), address(staking), weth.balanceOf(address(staking)) + 1 ether);

        vm.startPrank(alice);
        staking.claim();
        vm.stopPrank();
        vm.startPrank(bob);
        staking.claim();
        vm.stopPrank();
        // @audit Although Bob staking 100 blocks, Alice only needed to frontrun to get the same reward
        assertEq(weth.balanceOf(alice), weth.balanceOf(bob));
    }
}

Impact

Frontrun can get the full reward, which harms the interests of the staking users.

Tools Used

Foundry

Recommendations

Use time-weighted reward allocation algorithm.

Unchecked trasnfer/trasnferFrom may lead to silent returns

Unchecked trasnfer/trasnferFrom may lead to silent returns

Severity

Medium Risk

Relevant GitHub Links

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

Summary

Not using the safe version of transfer and transferFrom may lead to loss of funds, due to silent fails.

Vulnerability Details

Some tokens comply to the ERC20 interface and others don't. Because of this it's best to always check the return of transfer or trasnferFrom or even better, to use the safe version.

Example scenario is if we use USDT - it returns false on failed transfers, and does not revert. So any not checked transfer could fail silently and the code in Bleedle to continue executing without the funds being sent.

Impact

Loss of funds for the system and the users.

Tools Used

Manual review

Recommendations

Use safeTransfer and safeTransferFrom from SafeERC20.sol

Single-step ownership transfer pattern is dangerous

Single-step ownership transfer pattern is dangerous

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Ownable.sol

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

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

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

Summary

In utils the ownable.sol file is using the single-step ownership transfer pattern.

Vulnerability Details

Inheriting Ownable contract means you are using a single-step ownership transfer pattern. If an admin provides an incorrect address for the new owner this will result in none of the onlyOwner marked methods being callable again. The better way to do this is to use a two-step ownership transfer approach, where the new owner should first claim its new rights before they are transferred.

Impact

1.If an admin provides an incorrect address for the new owner this will result in none of the onlyOwner marked methods being callable again.
2.Funds gets stuck permanently.

Tools Used

Manual

Recommendations

Add AcceptOwnership method so that firstly the new owner claims the rights and after that old owner gets unauthorized.

Take refrence form openzeppelin Ownable2step contract:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

Profits can get stuck in Fees contract -- in case there's no 0.3% pool

Profits can get stuck in Fees contract -- in case there's no 0.3% pool

Severity

High Risk

Relevant GitHub Links

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

Summary

Profits can get stuck in Fees contract, in case there' no 0.3% pool to swap in.

Vulnerability Details

The _sellProfits function is only way to sell profits in the contract to WETH. However, the swap in UniV3 Router forces the swap path to be via 0.3% pool of the profit token and WETH, which may not exist (or have really low liquidity). This means the swap can have very very price impact, or not even possible at all if the pool doesn't exist --> so the profits will get stuck in the contract forever.

Impact

Either

  • Really high price impact if the underlying pool has really low liquidity, OR
  • Profits get stuck in the contract forever

Tools Used

Manual Review

Recommendations

  • Not fix the swap path to the 0.3% pool, or add certain checks to verify the existence of the 0.3% pool.
  • Add other ways to withdraw profit tokens in case the swap cannot happen.

Possible rounding issue

Possible rounding issue

Severity

Low Risk

Relevant GitHub Links

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

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

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

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

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

Summary

Vulnerability Details

Division by large numbers may result in the result being zero, due to solidity not supporting fractions.

file: Staking.sol

68: uint256 _ratio = _diff * 1e18 / totalSupply;
88: uint256 _share = _supplied * _delta / 1e18;
file: Lender.sol

561: uint256 govFee = (borrowerFee * loan.collateral) / 10000;
650: uint256 fee = (borrowerFee * (debt - debtToPay)) / 10000;
724: interest = (l.interestRate * l.debt * timeElapsed) / 10000 / 365 days;

Impact

When dividing by large numbers,the quotient may turn out to be zero.

Tools Used

Manual

Recommendations

Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator

Solidity version `0.8.20` may not work on other chains due to `PUSH0`

Solidity version 0.8.20 may not work on other chains due to PUSH0

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/interfaces/IERC20.sol#L2

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/interfaces/ISwapRouter.sol#L2

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Errors.sol#L2

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Ownable.sol#L2

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Structs.sol#L2

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

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

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

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

Summary

Vulnerability Details

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code.

Impact

This op code may not yet be implemented on all L2s, so deployment on these chains will fail.

Tools Used

Recommendations

To work around this issue, use an earlier EVM version. While the project itself may or may not compile with 0.8.20, other projects with which it integrates, or which extend this project may, and those projects will have problems deploying these contracts/libraries.

Missing Event emission in multiple functions

Missing Event emission in multiple functions

Severity

Low Risk

Vulnerability Details

The issue lies in Lender.sol where multiple functionalities do not emit an event. Emitting events is essential for notifying external entities such as users, chain explorers, or dApps about changes or conditions in the blockchain. Events, when emitted, store the arguments passed in transaction logs, making them accessible on the blockchain as long as the contract remains present.

Issue Location:

Lender.sol - Lines 84-87
Lender.sol - Lines 92-95
Lender.sol - Lines 100-102
Lender.sol - Lines 437-459

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

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

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

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

Impact
The emission of events is crucial to inform the off-chain world about successful state transitions. For the system's credibility and confidence, it is essential that administration functionalities emit the corresponding events throughout the system's lifecycle.

Tools Used

Manual Review

Recommendations

To address this vulnerability, it is strongly advised to emit an event for these functions.

`sellProfits` is prone to MEV sandwich attack

sellProfits is prone to MEV sandwich attack

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Fees.sol#L30-L40

Summary

sellProfits is prone to MEV sandwich attack --> profits can get drained

Vulnerability Details

The swap via UniswapV3 router has amountOutMinimum set to 0. And since the function is publicly callable by anyone, an attacker can MEV sandwich attack this sellProfits transaction initiated by the attacker, to reap the rewards.

Impact

Profits get MEV drained --> 0 WETH from the swap.

Tools Used

Manual Review

Recommendations

  • Make this function authorized and callable only by trusted parties.

transferFrom function can fail silently leading to wrong pool.poolBalance. An attacker can become lender and remove or borrow the tokens without sending any funds to the contract for lending.

transferFrom function can fail silently leading to wrong pool.poolBalance. An attacker can become lender and remove or borrow the tokens without sending any funds to the contract for lending.

Severity

High Risk

Relevant GitHub Links

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

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

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

Summary

Some ERC20 tokens doesn't revert on failure. It just returns false.
This can lead to wrong state updates of pool.poolBalance in the pool, which can lead to loss of funds.

Vulnerability Details

In setPool(Pool calldata p) function -> the transferFrom function can fail silently if the erc20 token doesnot revert on failure and just returns false.
Same case in addToPool(bytes32 poolId, uint256 amount) function. This can lead to wrong pool.poolBalance in the contract.

Impact

An attacker can become a lender and call setPool or addToPool without approving the contract to transfer erc20 tokens.
The transferFrom function will fail silenty and the subsequent lines will execute. The pool.PoolBalance will be set to the 'amount' parameter in the function.
The attacker now can withdraw from the pool using removeFromPool(bytes32 poolId, uint256 amount) without sending the tokens to the contract in the first place
The attacker can also use another wallet to borrow debt using borrow function. This will drain the contract.

Tools Used

Manual review

Recommendations

Use safeTransfer function from Openzeppelin. Some erc20 returns false even on success. So whitelisting specific tokens for use in the protocol is also recommended.

`<x> += <y>` costs more gas than `<x> = <x> + <y>` for state variables

Unauthorized Fund Claim in Staking Contract

Unauthorized Fund Claim in Staking Contract

Severity

Medium Risk

Relevant GitHub Links

TKN.transferFrom(msg.sender, address(this), _amount);

Summary

the operation is not properly secured, and there are no checks to ensure the sender is authorized to perform this action or validate if the transferred funds were staked or deposited as required.

Vulnerability Details

TKN.transferFrom(msg.sender, address(this), _amount);, here the contract isn't using safeERC20. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements or use safe wrapper functions implementing return value/data checks to handle these failures. For reference, https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom. Since there is no check for return status, the functions is moving forwarded to add balance for staking.

Impact

Attacker can steal WETH funds from the contract without fulfilling the staking requirements. the attacker can initiate unauthorized transfers and drain the staked funds.

Tools Used

Recommendations

Use safeERC20 or check balance of TKN after transferFrom

Single-step Ownership Transfer Can be Dangerous

Single-step Ownership Transfer Can be Dangerous

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Ownable.sol

Summary

Single-step Ownership Transfer Can be Dangerous

Vulnerability Details

Single-step ownership transfer means that if a wrong address was passed when transferring ownership or admin rights it can mean that role is lost forever. If the admin permissions are given to the wrong address within this function, it will cause irreparable damage to the contract.

The control of contract ownership is implemented similarly to the Ownable contract from OpenZeppelin. However, it should be noted that the OpenZeppelin documentation mentions that this approach is not secure.
https://docs.openzeppelin.com/contracts/4.x/api/access

"Ownable is a simpler mechanism with a single owner "role" that can be assigned to a single account. This simpler mechanism can be useful for quick tests but projects with production concerns are likely to outgrow it."

Impact

Allowing the protocol to cause irreparable losses results in the loss of ownership and destruction.

Tools Used

Manual review

Recommendations

It is a best practice to use a two-step ownership transfer pattern, meaning ownership transfer gets to a "pending" state and the new owner should claim his new rights, otherwise the old owner still has control of the contract.You can refer to OpenZeppelin's implementation for reference.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

Using `unchecked` in `for` loops can save plenty of gas

Using unchecked in for loops can save plenty of gas

Severity

Gas Optimization

Relevant GitHub Links

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

Summary

In the Lender.sol contract,unchecked can be used to reduce a significant amount of gas by adding the increment of counter i in a for loop under an unchecked section.

Vulnerability Details

In cases when we know that a certain number cannot go below or beyond a certain limit, implementing unchecked is good to reduce gas usage. For instance, you can find the gas usage by my deployment on sepolia network, the as-is deployment consumed about 3,399,220 gas, whereas, when I implemented the unchecked method for a single function, startAuction() function. The gas dropped to 3,396,784 which is a difference of 2436 gas.

Original Contract

Contract with just a single changed instance of the optimization

There are a total of 6 functions using for loops, namely: borrow(), repay(), giveLoan(), startAuction(), seizeLoan(), and refinance(). Implementing this change for each function will mean saving tx. gas of about:

6 * 2436 = 14616 gas

Impact

Considerable save on gas.

Tools Used

  • Manual audit
  • Foundry

Recommendations

You can find my implementation on the testnet too. Implementing this simple change as shown below can help serve the impact:

function startAuction(uint256[] calldata loanIds) public {
        for (uint256 i = 0; i < loanIds.length; ) {
            uint256 loanId = loanIds[i];
            // get the loan info
            Loan memory loan = loans[loanId];
            // validate the loan
            if (msg.sender != loan.lender) revert Unauthorized();
            if (loan.auctionStartTimestamp != type(uint256).max)
                revert AuctionStarted();

            // set the auction start timestamp
            loans[loanId].auctionStartTimestamp = block.timestamp;
            emit AuctionStart(
                loan.borrower,
                loan.lender,
                loanId,
                loan.debt,
                loan.collateral,
                block.timestamp,
                loan.auctionLength
            );
            unchecked {
                ++i;
            }
        }
    }

Re-entrancy on addToPool and Unhandled return values on addToPool/borrow/repay functions

Re-entrancy on addToPool and Unhandled return values on addToPool/borrow/repay functions

Severity

High Risk

Relevant GitHub Links

IERC20(pools[poolId].loanToken).transferFrom(

IERC20(loan.loanToken).transfer(feeReceiver, fees);
// transfer the loan tokens from the pool to the borrower
IERC20(loan.loanToken).transfer(msg.sender, debt - fees);
// transfer the collateral tokens from the borrower to the contract
IERC20(loan.collateralToken).transferFrom(
msg.sender,
address(this),
collateral
);

IERC20(loan.loanToken).transferFrom(
msg.sender,
address(this),
loan.debt + lenderInterest
);
// transfer the protocol fee to the fee receiver
IERC20(loan.loanToken).transferFrom(
msg.sender,
feeReceiver,
protocolInterest
);
// transfer the collateral tokens from the contract to the borrower
IERC20(loan.collateralToken).transfer(
loan.borrower,
loan.collateral
);

Summary

ERC20 external call in the addToPool function of the Lender contract is vulnerable to re-entrancy attacks and unhandled return values for transfer and transferFrom functions. This vulnerability allows an attacker to manipulate the pool balance and cause losses to borrowers' collateral funds. The issue arises due to improper handling of the loanToken and collateralToken transfer operations.

Vulnerability Details

Untrusted pools leads to re-entrancy and poolbalance manipulation :

IERC20(pools[poolId].loanToken).transferFrom(

The addToPool function doesn't include any re-entrancy guard, allowing malicious contracts to potentially call this function repeatedly before it completes execution, leading to unexpected behaviour and manipulation of the poolBalance.
In detail, here the attacker will skip/erase transferFrom functionality in his own malicious contract and re-entered into function and manipulate PoolBalance.

IERC20(loan.loanToken).transfer(feeReceiver, fees);
// transfer the loan tokens from the pool to the borrower
IERC20(loan.loanToken).transfer(msg.sender, debt - fees);
// transfer the collateral tokens from the borrower to the contract
IERC20(loan.collateralToken).transferFrom(
msg.sender,
address(this),
collateral
);

IERC20(loan.loanToken).transferFrom(
msg.sender,
address(this),
loan.debt + lenderInterest
);
// transfer the protocol fee to the fee receiver
IERC20(loan.loanToken).transferFrom(
msg.sender,
feeReceiver,
protocolInterest
);
// transfer the collateral tokens from the contract to the borrower
IERC20(loan.collateralToken).transfer(
loan.borrower,
loan.collateral
);

here, also malformed transfer function skip transfering FTK to user (if he wants to, he it can gave him access to re-enter but no use with reentrancy here) and client/borrower's loss his collateral holdings and it may stuck inside the contract.
POC (borrow fun):
loadToken - ("FakeToken", FTK)
collateralToken - ("USD-Zebra", USDz)
debt = 20 FTK
collateral = 20 USDz
IERC20(loan.loanToken).transfer(feeReceiver, fees);
// transfer the loan tokens from the pool to the borrower
IERC20(loan.loanToken).transfer(msg.sender, debt - fees);
// transfer the collateral tokens from the borrower to the contract
IERC20(loan.collateralToken).transferFrom(
msg.sender,
address(this),
collateral
);

the malicious-contract stored in loadToken, can skip the transfer, but borrower looses the collateral since he dont receive FTK.

Impact

  1. pool balance will be shown very higher than originally it has.
  2. borrower losses his collateral funds

Tools Used

Recommendations

loanToken address is always decided by the contract owner right in this case the loantoken.
use OpenZeppelin’s SafeERC20 wrapper functions and Re-entrancy guard.

bool result = IERC20(pools[poolId].loanToken).safeTransferFrom( msg.sender, address(this), pools[poolId].poolBalance + amount ); require(result,"Error"); // Update pool here after successfull transfer

The `owner` is a single point of failure and a centralization risk

The owner is a single point of failure and a centralization risk

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/utils/Ownable.sol#L19

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

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

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

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

Summary

The owner role has a single point of failure and onlyOwner can use critical functions, posing a centralization issue.

Vulnerability Details

Having a single EOA as the only owner of contracts is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary.

Code Snippet

file: Ownable.col

19: function transferOwnership(address _owner) public virtual onlyOwner {
file: Beedle.sol

36: function mint(address to, uint256 amount) external onlyOwner {
file: Lender.sol

84: function setLenderFee(uint256 _fee) external onlyOwner {
92: function setBorrowerFee(uint256 _fee) external onlyOwner {
100: function setFeeReceiver(address _feeReceiver) external onlyOwner {

Impact

There is always a chance for owner keys to be stolen, and in such a case, the attacker can cause damage to the project due to important functions.

Tools Used

Manual

Recommendations

Consider changing to a multi-signature setup, or having a role-based authorization model.

MEV can sandwich every harvest due to missing slippage tolerance value

MEV can sandwich every harvest due to missing slippage tolerance value

Severity

Medium Risk

Relevant GitHub Links

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

Summary

MEV can sandwich every Profit due to missing slippage tolerance value

Vulnerability Details

In fees.sol every time sellProfits methods was called the following code gets executed:

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

The “0” here is the value of the amountOutMinimum argument which is used for slippage tolerance. 0 value here essentially means 100% slippage tolerance. This is a very easy target for MEV and bots to do a flash loan sandwich attack on each of the strategy’s swaps, resulting in very big slippage on each trade.

Impact

100% slippage tolerance can be exploited in a way that the strategy (so the profit and the users) receive much less value than it should had. This can be done on every trade if the trade transaction goes through a public mempool.

Tools Used

Manual Test

Recommendations

The best solution here is to make the sellProfits method of the vault be callable only by a list of trusted addresses which will send the transaction through a private mempool. This, combined with an on-chain calculation for an amountOutMinium that is off from the expected amount out by a slippage tolerance percentage (that might be configurable through a setter in the strategy) should be good enough to protect you from MEV sandwich attacks.

Missing zero check address in constructor Staking.sol

Missing zero check address in constructor Staking.sol

Severity

Low Risk

Relevant GitHub Links

constructor(address _token, address _weth) Ownable(msg.sender) {
TKN = IERC20(_token);
WETH = IERC20(_weth);
}

Summary

In Staking.sol have set TKN and WETH addresses but there no have check for zero addresses. If deoplyer by mistake or error call constructor without set addresses then the protocol can't work.

Vulnerability Details

TKN and WETH missing zero check addresses

Impact

If TKN and/or WETH address is missing there no have check for that and the whole protocol won't work.

Tools Used

Manual review

Recommendations

require(address _token != address(0));
require(address _weth != address(0));

`++i/i++` should be `unchecked{++i}/unchecked{i++}` when it is not possible for them to overflow, as is the case when used in for- and while-loops

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

Severity

Gas Optimization

Relevant GitHub Links

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

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

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

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

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

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

Summary

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

Vulnerability Details

Impact

Gas Consumption

Tools Used

Recommendations

Prevent division by 0

Prevent division by 0

Severity

Low Risk

Relevant GitHub Links

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

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

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

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

Summary

On several locations in the code precautions are not being taken for not dividing by 0, this will revert the code.

Vulnerability Details

These functions can be called with 0 value in the input, this value is not checked for being bigger than 0, that means in some scenarios this can potentially trigger a division by zero.

Code Snippet

File: Lender.sol

265: uint256 fees = (debt * borrowerFee) / 10000;
561: uint256 govFee = (borrowerFee * loan.collateral) / 10000;
650: uint256 fee = (borrowerFee * (debt - debtToPay)) / 10000;
725: fees = (lenderFee * interest) / 10000;

Impact

In certain cases, this could lead to a division by zero.

Tools Used

Manual

Recommendations

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.

x += y/x -= y costs more gas than x = x + y/x = x - y for state variables

Use safeTransfer/safeTransferFrom instead of transfer/transferFrom

Use safeTransfer/safeTransferFrom instead of transfer/transferFrom

Severity

Medium Risk

Relevant GitHub Links

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Summary

Use safeTransfer/safeTransferFrom instead of transfer/transferFrom.

Vulnerability Details

Code does not check return values of transfer/transferFrom, and non-standard tokens may not return true even though the transfer succeeded.

Impact

Code may happily execute assuming a transfer succeeded when it actually failed.

Tools Used

Manual

Recommendations

Use safeTransfer/safeTransferFrom instead of transfer/transferFrom.

Using storage instead of memory for structs/arrays saves gas

Using storage instead of memory for structs/arrays saves gas

Severity

Gas Optimization / Informational

Relevant GitHub Links

https://github.com/code-423n4/2023-01-ondo-findings/blob/main/report.md#g-09-using-storage-instead-of-memory-for-structsarrays-saves-gas

Summary

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

Vulnerability Details

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

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

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

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

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

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

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

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

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

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

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

Tools Used

Manual

Recommendations

  1. Use storage for those structs/arrays.

Lack of specific time input can result in MEVs exploiting `sellProfits`

Lack of specific time input can result in MEVs exploiting sellProfits

Severity

Medium Risk

Relevant GitHub Links

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

Summary

Lack of time specific input can result in MEVs exploiting the sellProfits TX.

Vulnerability Details

From the code snippet bellow we can see that for the UNI call the deadline parameter is set to block.timestamp. Deadline is where is the last time that the TX can be executed, and any time after it it revert. The issue here is that block.timestamp can be easily manipulated by the MEVs (they can include it in this block,top or bottom, or even pay the block proposers to put it in the next block). This all means that the deadline is useless, since block.timestamp is when the TX is executed and no matter when it's executed it's gonna pass (now, a day later, or even a year).

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

Impact

MEVs, could game the sellProfits TX.

Tools Used

Recommendations

Set a deadline settle in the function.

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

`<array>.length` Should Not Be Looked Up In Every Loop Of A For-loop

<array>.length Should Not Be Looked Up In Every Loop Of A For-loop

Severity

Gas Optimization

Relevant GitHub Links

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

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

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

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

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

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

Summary

Vulnerability Details

The overheads outlined below are PER LOOP, excluding the first loop

  • storage arrays incur a Gwarmaccess (100 gas)
  • memory arrays use MLOAD (3 gas)
  • calldata arrays use CALLDATALOAD (3 gas)
    Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP needed to store the stack offset

Impact

Gas Consumption

Tools Used

Recommendations

Fees.sellProfits() swap with deadline: block.timestamp offers no protection

Fees.sellProfits() swap with deadline: block.timestamp offers no protection

Severity

Medium Risk

Relevant GitHub Links

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

Summary

Fees.sellProfits() swap with deadline: block.timestamp offers no protection.

Vulnerability Details

Whichever block the txn will be included in will be block.timestamp, so setting the deadline to block.timestamp offers no protection as validators can hold the transaction indefinitely.

Impact

The transaction can be held indefinitely for MEV until it results in maximum slippage.

Tools Used

Manual

Recommendations

Allow function caller to specify deadline parameter.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.