LOW

[codehawks-bot]

LOW

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle

Low Risk Issues

Number	Issue	Instances
[LOW-01]	Use Ownable2Step rather than Ownable	4
[LOW-02]	Functions which set address state variables should have zero address checks	2
[LOW-03]	Unbounded state variable arrays exist within the project which are iterated upon	1
[LOW-04]	Potential division by zero should have zero checks in place	3
[LOW-05]	Contracts do not use their OZ upgradable counterparts	3
[LOW-06]	Use of onlyOwner functions can be lost	4
[LOW-07]	Constant decimal values	3
[LOW-08]	Functions calling contracts/addresses with transfer hooks are missing reentrancy guards	12
[LOW-09]	Calculation will revert when totalSupply() returns zero	1
[LOW-10]	Missing zero address check in constructor	3

[LOW‑01] Use Ownable2Step rather than Ownable

Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.

File: src/Beedle.sol

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

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

File: src/Lender.sol

10   contract Lender is Ownable {

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

File: src/Staking.sol

11   contract Staking is Ownable {

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

File: src/utils/Ownable.sol

4   abstract contract Ownable {

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

[LOW-02] Functions which set address state variables should have zero address checks

File:   src/Lender.sol

100       function setFeeReceiver(address _feeReceiver) external onlyOwner {
        feeReceiver = _feeReceiver;
    }

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

File:   src/utils/Ownable.sol

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

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

[LOW-03] Unbounded state variable arrays exist within the project which are iterated upon

Unbounded arrays in Solidity can cause gas-related issues due to their potential for excessive growth, leading to increased computational complexity and resource consumption. Since Ethereum's gas fees are determined by the amount of computational effort required to execute a transaction, operations involving large, unbounded arrays can result in prohibitively high costs for users. Additionally, if a function iterates over an unbounded array, it may exceed the gas limit set by the network, causing the transaction to fail. To avoid these issues, developers should use bounded arrays or alternative data structures like mappings, ensuring efficient resource management and a better user experience.

file:  src/Lender.sol

276   loans.push(loan);

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

[LOW-04] Potential division by zero should have zero checks in place

Implement a zero address check for found instances

File:  src/Staking.sol

68      uint256 _ratio = _diff * 1e18 / totalSupply;

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

File:  src/Lender.sol

246    uint256 loanRatio = (debt * 10 ** 18) / collateral;

384   uint256 loanRatio = (totalDebt * 10 ** 18) / loan.collateral;

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

[LOW-05] Contracts do not use their OZ upgradable counterparts

Using the upgradeable counterpart of the OpenZeppelin (OZ) library in Solidity is beneficial for creating contracts that can be updated in the future. OpenZeppelin's upgradeable contracts library is designed with proxy patterns in mind, which allow the logic of contracts to be upgraded while preserving the contract's state and address. This can be crucial for long-lived contracts where future requirements or improvements may not be fully known at the time of deployment. The upgradeable OZ contracts also include protection against a class of vulnerabilities related to initialization of storage variables in upgradeable contracts. Hence, it's a good idea to use them when developing contracts that may need to be upgraded in the future, as they provide a solid foundation for secure and upgradeable smart contracts.

File: src/Beedle.sol

5     import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
6     import {ERC20Permit} from "openzeppelin-contracts/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
7     import {ERC20Votes} from "openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20Votes.sol";

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

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

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

[LOW-06] Use of onlyOwner functions can be lost

In Solidity, renouncing ownership of a contract essentially transfers ownership to the zero address. This is an irreversible operation and has considerable security implications. If the renounceOwnership function is used, the contract will lose the ability to perform any operations that are limited to the owner. This can be problematic if there are any bugs, flaws, or unexpected events that require owner intervention to resolve. Therefore, in some instances, it is better to disable or omit the renounceOwnership function, and instead implement a secure transferOwnership function. This way, if necessary, ownership can be transferred to a new, trusted party without losing the potential for administrative intervention.

File: src/Beedle.sol

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

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

File: src/Lender.sol

10   contract Lender is Ownable {

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

File: src/Staking.sol

11   contract Staking is Ownable {

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

File: src/utils/Ownable.sol

4   abstract contract Ownable {

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

[LOW-07] Constant decimal values

The use of fixed decimal values such as 1e18 or 1e8 in Solidity contracts can lead to inaccuracies, bugs, and vulnerabilities, particularly when interacting with tokens having different decimal configurations. Not all ERC20 tokens follow the standard 18 decimal places, and assumptions about decimal places can lead to miscalculations.

Resolution: Always retrieve and use the decimals() function from the token contract itself when performing calculations involving token amounts. This ensures that your contract correctly handles tokens with any number of decimal places, mitigating the risk of numerical errors or under/overflows that could jeopardize contract integrity and user funds

File:  src/Staking.sol

88    uint256 _share = _supplied * _delta / 1e18;

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

File:  src/Beedle.sol

12   _mint(msg.sender, 1_000_000_000 * 1e18);

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

File:  src/Staking.sol

68    uint256 _ratio = _diff * 1e18 / totalSupply;

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

[LOW-08] Functions calling contracts/addresses with transfer hooks are missing reentrancy guards

While adherence to the check-effects-interaction pattern is commendable, the absence of a reentrancy guard in functions, especially where transfer hooks might be present, can expose the protocol users to risks of read-only reentrancies. Such reentrancy vulnerabilities can be exploited to execute malicious actions even without altering the contract state. Without a reentrancy guard, the only potential mitigation would be to blocklist the entire protocol - an extreme and disruptive measure. Therefore, incorporating a reentrancy guard into these functions is vital to bolster security, as it helps protect against both traditional reentrancy attacks and read-only reentrancies, ensuring robust and safe protocol operations.

File:  src/Beedle.sol

15       function _afterTokenTransfer(address from, address to, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._afterTokenTransfer(from, to, amount);  // <= FOUND
    }

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

File:   src/Staking.sol

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

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

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

File:  src/Fees.sol

26       function sellProfits(address _profits) public {
        require(_profits != WETH, "not allowed");
        uint256 amount = IERC20(_profits).balanceOf(address(this));

        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)));   // <= FOUND
    }

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

File:   src/Lender.sol

130       function setPool(Pool calldata p) public returns (bytes32 poolId) {
        // validate the pool
        if (
            p.lender != msg.sender ||
            p.minLoanSize == 0 ||
            p.maxLoanRatio == 0 ||
            p.auctionLength == 0 ||
            p.auctionLength > MAX_AUCTION_LENGTH ||
            p.interestRate > MAX_INTEREST_RATE
        ) revert PoolConfig();

        // check if they already have a pool balance
        poolId = getPoolId(p.lender, p.loanToken, p.collateralToken);

        // you can't change the outstanding loans
        if (p.outstandingLoans != pools[poolId].outstandingLoans)
            revert PoolConfig();

        uint256 currentBalance = pools[poolId].poolBalance;

        if (p.poolBalance > currentBalance) {
            // if new balance > current balance then transfer the difference from the lender
            IERC20(p.loanToken).transferFrom(    // <= FOUND
                p.lender,
                address(this),
                p.poolBalance - currentBalance
            );
        } else if (p.poolBalance < currentBalance) {
            // if new balance < current balance then transfer the difference back to the lender
            IERC20(p.loanToken).transfer(      // <= FOUND
                p.lender,
                currentBalance - p.poolBalance
            );
        }

        emit PoolBalanceUpdated(poolId, p.poolBalance);

        if (pools[poolId].lender == address(0)) {
            // if the pool doesn't exist then create it
            emit PoolCreated(poolId, p);
        } else {
            // if the pool does exist then update it
            emit PoolUpdated(poolId, p);
        }

        pools[poolId] = p;
    }

198       function removeFromPool(bytes32 poolId, uint256 amount) external {
        if (pools[poolId].lender != msg.sender) revert Unauthorized();
        if (amount == 0) revert PoolConfig();
        _updatePoolBalance(poolId, pools[poolId].poolBalance - amount);
        // transfer the loan tokens from the contract to the lender
        IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);   // <= FOUND
    }

232      function borrow(Borrow[] calldata borrows) public {
        for (uint256 i = 0; i < borrows.length; i++) {
            bytes32 poolId = borrows[i].poolId;
            uint256 debt = borrows[i].debt;
            uint256 collateral = borrows[i].collateral;
            // get the pool info
            Pool memory pool = pools[poolId];
            // make sure the pool exists
            if (pool.lender == address(0)) revert PoolConfig();
            // validate the loan
            if (debt < pool.minLoanSize) revert LoanTooSmall();
            if (debt > pool.poolBalance) revert LoanTooLarge();
            if (collateral == 0) revert ZeroCollateral();
            // make sure the user isn't borrowing too much
            uint256 loanRatio = (debt * 10 ** 18) / collateral;
            if (loanRatio > pool.maxLoanRatio) revert RatioTooHigh();
            // create the loan
            Loan memory loan = Loan({
                lender: pool.lender,
                borrower: msg.sender,
                loanToken: pool.loanToken,
                collateralToken: pool.collateralToken,
                debt: debt,
                collateral: collateral,
                interestRate: pool.interestRate,
                startTimestamp: block.timestamp,
                auctionStartTimestamp: type(uint256).max,
                auctionLength: pool.auctionLength
            });
            // update the pool balance
            _updatePoolBalance(poolId, pools[poolId].poolBalance - debt);
            pools[poolId].outstandingLoans += debt;
            // calculate the fees
            uint256 fees = (debt * borrowerFee) / 10000;
            // transfer fees
            IERC20(loan.loanToken).transfer(feeReceiver, fees);     // <= FOUND
            // transfer the loan tokens from the pool to the borrower
            IERC20(loan.loanToken).transfer(msg.sender, debt - fees);    // <= FOUND
            // transfer the collateral tokens from the borrower to the contract
            IERC20(loan.collateralToken).transferFrom(     // <= FOUND
                msg.sender,
                address(this),
                collateral
            );
            loans.push(loan);
            emit Borrowed(
                msg.sender,
                pool.lender,
                loans.length - 1,
                debt,
                collateral,
                pool.interestRate,
                block.timestamp
            );
        }
    }

292       function repay(uint256[] calldata loanIds) public {
        for (uint256 i = 0; i < loanIds.length; i++) {
            uint256 loanId = loanIds[i];
            // get the loan info
            Loan memory loan = loans[loanId];
            // calculate the interest
            (
                uint256 lenderInterest,
                uint256 protocolInterest
            ) = _calculateInterest(loan);

            bytes32 poolId = getPoolId(
                loan.lender,
                loan.loanToken,
                loan.collateralToken
            );

            // update the pool balance
            _updatePoolBalance(
                poolId,
                pools[poolId].poolBalance + loan.debt + lenderInterest
            );
            pools[poolId].outstandingLoans -= loan.debt;

            // transfer the loan tokens from the borrower to the pool
            IERC20(loan.loanToken).transferFrom(       // <= FOUND
                msg.sender,
                address(this),
                loan.debt + lenderInterest
            );
            // transfer the protocol fee to the fee receiver
            IERC20(loan.loanToken).transferFrom(      // <= FOUND
                msg.sender,
                feeReceiver,
                protocolInterest
            );
            // transfer the collateral tokens from the contract to the borrower
            IERC20(loan.collateralToken).transfer(      // <= FOUND
                loan.borrower,   
                loan.collateral
            );
            emit Repaid(
                msg.sender,
                loan.lender,
                loanId,
                loan.debt,
                loan.collateral,
                loan.interestRate,
                loan.startTimestamp
            );
            // delete the loan
            delete loans[loanId];
        }
    }

591       function refinance(Refinance[] calldata refinances) public {
        for (uint256 i = 0; i < refinances.length; i++) {
            uint256 loanId = refinances[i].loanId;
            bytes32 poolId = refinances[i].poolId;
            bytes32 oldPoolId = keccak256(
                abi.encode(
                    loans[loanId].lender,
                    loans[loanId].loanToken,
                    loans[loanId].collateralToken
                )
            );
            uint256 debt = refinances[i].debt;
            uint256 collateral = refinances[i].collateral;

            // get the loan info
            Loan memory loan = loans[loanId];
            // validate the loan
            if (msg.sender != loan.borrower) revert Unauthorized();

            // get the pool info
            Pool memory pool = pools[poolId];
            // validate the new loan
            if (pool.loanToken != loan.loanToken) revert TokenMismatch();
            if (pool.collateralToken != loan.collateralToken)
                revert TokenMismatch();
            if (pool.poolBalance < debt) revert LoanTooLarge();
            if (debt < pool.minLoanSize) revert LoanTooSmall();
            uint256 loanRatio = (debt * 10 ** 18) / collateral;
            if (loanRatio > pool.maxLoanRatio) revert RatioTooHigh();

            // calculate the interest
            (
                uint256 lenderInterest,
                uint256 protocolInterest
            ) = _calculateInterest(loan);
            uint256 debtToPay = loan.debt + lenderInterest + protocolInterest;

            // update the old lenders pool
            _updatePoolBalance(
                oldPoolId,
                pools[oldPoolId].poolBalance + loan.debt + lenderInterest
            );
            pools[oldPoolId].outstandingLoans -= loan.debt;

            // now lets deduct our tokens from the new pool
            _updatePoolBalance(poolId, pools[poolId].poolBalance - debt);
            pools[poolId].outstandingLoans += debt;

            if (debtToPay > debt) {
                // we owe more in debt so we need the borrower to give us more loan tokens
                // transfer the loan tokens from the borrower to the contract
                IERC20(loan.loanToken).transferFrom(     // <= FOUND
                    msg.sender,
                    address(this),
                    debtToPay - debt
                );
            } else if (debtToPay < debt) {
                // we have excess loan tokens so we give some back to the borrower
                // first we take our borrower fee
                uint256 fee = (borrowerFee * (debt - debtToPay)) / 10000;
                IERC20(loan.loanToken).transfer(feeReceiver, fee);             // <= FOUND
                // transfer the loan tokens from the contract to the borrower
                IERC20(loan.loanToken).transfer(msg.sender, debt - debtToPay - fee);     // <= FOUND
            }
            // transfer the protocol fee to governance
            IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);       // <= FOUND

            // update loan debt
            loans[loanId].debt = debt;
            // update loan collateral
            if (collateral > loan.collateral) {
                // transfer the collateral tokens from the borrower to the contract
                IERC20(loan.collateralToken).transferFrom(
                    msg.sender,
                    address(this),
                    collateral - loan.collateral
                );
            } else if (collateral < loan.collateral) {
                // transfer the collateral tokens from the contract to the borrower
                IERC20(loan.collateralToken).transfer(          // <= FOUND
                    msg.sender,
                    loan.collateral - collateral
                );
            }

            emit Repaid(
                msg.sender,
                loan.lender,
                loanId,
                debt,
                collateral,
                loan.interestRate,
                loan.startTimestamp
            );

            loans[loanId].collateral = collateral;
            // update loan interest rate
            loans[loanId].interestRate = pool.interestRate;
            // update loan start timestamp
            loans[loanId].startTimestamp = block.timestamp;
            // update loan auction start timestamp
            loans[loanId].auctionStartTimestamp = type(uint256).max;
            // update loan auction length
            loans[loanId].auctionLength = pool.auctionLength;
            // update loan lender
            loans[loanId].lender = pool.lender;
            // update pool balance
            pools[poolId].poolBalance -= debt;
            emit Borrowed(
                msg.sender,
                pool.lender,
                loanId,
                debt,
                collateral,
                pool.interestRate,
                block.timestamp
            );
            emit Refinanced(loanId);
        }
    }

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

[LOW-09] Calculation will revert when totalSupply() returns zero

In the instance where the function totalSupply() returns zero, it will inevitably lead to a division by zero error when used in mathematical operations, causing the transaction to fail and potentially disrupting contract functionality. This situation can inadvertently serve as a blocking mechanism, preventing valid transactions and operations. It's crucial to accommodate this special scenario in your code. One resolution could be implementing condition checks in your function to detect a zero totalSupply() and handle it differently, perhaps by returning a specific value or altering the operational flow, thus ensuring that transactions do not revert and the contract functions smoothly even in this edge case.

File:  src/Staking.sol

68    uint256 _ratio = _diff * 1e18 / totalSupply;

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

[LOW-10] Missing zero address check in constructor

In Solidity, constructors often take address parameters to initialize important components of a contract, such as owner or linked contracts. However, without a check, there's a risk that an address parameter could be mistakenly set to the zero address (0x0). This could occur due to a mistake or oversight during contract deployment. A zero address in a crucial role can cause serious issues, as it cannot perform actions like a normal address, and any funds sent to it are irretrievable. Therefore, it's crucial to include a zero address check in constructors to prevent such potential problems. If a zero address is detected, the constructor should revert the transaction.

File:  src/Fees.sol

19       constructor(address _weth, address _staking) {
        WETH = _weth;
        staking = _staking;
    }

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

File:  src/Staking.sol

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

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

File:  src/utils/Ownable.sol

14      constructor(address _owner) {
        owner = _owner;
        emit OwnershipTransferred(address(0), _owner);
    }   

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

[PatrickAlphaC]

3. Is OK since it's in the parameters not storage
4. Checks are in place
5. Not upgradeable
6. Expected
7. Informational
8. Need proof of an issue
9. There is a check that totalsupply is greater than 0