search

Cyfrin / 2023-07-beedle

19 stars 19 forks source link

Low Risk Report #1315

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Low Risk Report

Severity

Low Risk

QA Summary

Low Risk Issues

Issue Contexts
LOW‑1 Consider checking that transfer to address(this) is disabled 3
LOW‑2 Constant decimal values 2
LOW‑3 Draft Import Dependencies 1
LOW‑4 External calls in an un-bounded for-loop may result in a DOS 4
LOW‑5 Lack of checks-effects-interactions 3
LOW‑6 Large transfers may not work with some ERC20 tokens 25
LOW‑7 Possible rounding issue 6
LOW‑8 Minting tokens to the zero address should be avoided 1
LOW‑9 Constructor is missing zero address check 2
LOW‑10 Missing Checks for Address(0x0) 1
LOW‑11 Contracts are not using their OZ Upgradeable counterparts 3
LOW‑12 Solidity version 0.8.20 may not work on other chains due to PUSH0 9
LOW‑13 TransferOwnership Should Be Two Step 1
LOW‑14 Unbounded loop 1
LOW‑15 Use Ownable2Step rather than Ownable 9
LOW‑16 Use safeTransferOwnership instead of transferOwnership function 3
LOW‑17 A year is not always 365 days 1

Total: 75 contexts over 17 issues

Low Risk Issues

[LOW‑1] Consider checking that transfer to address(this) is disabled

Functions allowing users to specify the receiving address should check whether the referenced address is not address(this). This would prevent tokens to remain lock inside the contract. An example of the potential for loss by leaving this open is the EOS token smart contract where more than 90,000 tokens are stuck at the contract address.

Proof Of Concept

File: Beedle.sol

22: function _mint(address to, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._mint(to, amount);
    }

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

File: Beedle.sol

29: function _burn(address account, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._burn(account, amount);
    }

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

File: Beedle.sol

36: function mint(address to, uint256 amount) external onlyOwner {
        _mint(to, amount);
    }

}

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

[LOW‑2] 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.

Proof Of Concept

File: Staking.sol

68: uint256 _ratio = _diff * 1e18 / totalSupply;

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

File: Staking.sol

88: uint256 _share = _supplied * _delta / 1e18;

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

[LOW‑3] Draft Import Dependencies

Contracts are importing draft dependencies. These imported contracts are still a draft and are not considered ready for mainnet use and have not received adequate security auditing or are liable to change with future development.

Proof Of Concept

File: Beedle.sol

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

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

Recommended Mitigation Steps

Stop using draft imported contracts and use their release version if it exists.

[LOW‑4] External calls in an un-bounded for-loop may result in a DOS

Consider limiting the number of iterations in for-loops that make external calls

Proof Of Concept

File: Lender.sol

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++) {

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

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

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

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

[LOW‑5] Lack of checks-effects-interactions

It's recommended to execute external calls after state changes, to prevent reentrancy bugs.

Consider moving the external calls after the state changes on the following instances.

Proof Of Concept

```solidity File: Lender.sol 187: function addToPool(bytes32 poolId, uint256 amount) external { if (pools[poolId].lender != msg.sender) revert Unauthorized(); if (amount == 0) revert PoolConfig(); _updatePoolBalance(poolId, pools[poolId].poolBalance + amount); IERC20(pools[poolId].loanToken).transferFrom( msg.sender, address(this), amount ); } ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L187 ```solidity File: Lender.sol 203: 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); IERC20(pools[poolId].loanToken).transfer(msg.sender, amount); } ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L203 ```solidity File: Staking.sol 49: function withdraw(uint _amount) external { updateFor(msg.sender); balances[msg.sender] -= _amount; TKN.transfer(msg.sender, _amount); } ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Staking.sol#L49

[LOW‑6] Large transfers may not work with some ERC20 tokens

Some IERC20 implementations (e.g UNI, COMP) may fail if the valued transferred is larger than uint96. Source

Proof Of Concept

```solidity File: Lender.sol 152: IERC20(p.loanToken).transferFrom( 159: IERC20(p.loanToken).transfer( ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L152 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L159 ```solidity File: Lender.sol 187: IERC20(pools[poolId].loanToken).transferFrom( ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L187 ```solidity File: Lender.sol 203: IERC20(pools[poolId].loanToken).transfer(msg.sender, amount); ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L203 ```solidity File: Lender.sol 267: IERC20(loan.loanToken).transfer(feeReceiver, fees); 269: IERC20(loan.loanToken).transfer(msg.sender, debt - fees); 271: IERC20(loan.collateralToken).transferFrom( 663: IERC20(loan.collateralToken).transferFrom( ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L267 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L269 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L271 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L663 ```solidity File: Lender.sol 317: IERC20(loan.loanToken).transferFrom( 323: IERC20(loan.loanToken).transferFrom( 642: IERC20(loan.loanToken).transferFrom( 329: IERC20(loan.collateralToken).transfer( 563: IERC20(loan.collateralToken).transfer( 565: IERC20(loan.collateralToken).transfer( 670: IERC20(loan.collateralToken).transfer( ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L317 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L323 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L642 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L329 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L563 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L565 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L670 ```solidity File: Lender.sol 403: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); 505: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); 656: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L403 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L505 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L656 ```solidity File: Lender.sol 403: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); 505: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); 656: IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest); ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L403 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L505 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L656 ```solidity File: Lender.sol 563: IERC20(loan.collateralToken).transfer(feeReceiver, govFee); 329: IERC20(loan.collateralToken).transfer( 565: IERC20(loan.collateralToken).transfer( 670: IERC20(loan.collateralToken).transfer( ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L563 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L329 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L565 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L670

[LOW‑7] Possible rounding issue

Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator

Proof Of Concept

```solidity File: Lender.sol 246: uint256 loanRatio = (debt * 10 ** 18) / collateral; 618: uint256 loanRatio = (debt * 10 ** 18) / collateral; 265: uint256 fees = (debt * borrowerFee) / 10000; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L246 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L618 https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L265 ```solidity File: Lender.sol 475: uint256 currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) / loan.auctionLength; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L475 ```solidity File: Lender.sol 561: uint256 govFee = (borrowerFee * loan.collateral) / 10000; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L561 ```solidity File: Lender.sol 724: interest = (l.interestRate * l.debt * timeElapsed) / 10000 / 365 days; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L724

[LOW‑8] Minting tokens to the zero address should be avoided

The core function mint is used by users to mint an option position by providing token1 as collateral and borrowing the max amount of liquidity. Address(0) check is missing in both this function and the internal function _mint, which is triggered to mint the tokens to the to address. Consider applying a check in the function to ensure tokens aren't minted to the zero address.

Proof Of Concept

File: Beedle.sol

function mint(address to, uint256 amount) external onlyOwner {
        _mint(to, amount);
    }

}

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

[LOW‑9] Constructor is missing zero address check

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.

Proof Of Concept

File: Fees.sol

19: constructor: address _weth

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

File: Ownable.sol

14: constructor: address _owner

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

[LOW‑10] Missing Checks for Address(0x0)

Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

Proof Of Concept

File: Lender.sol

100: function setFeeReceiver: address _feeReceiver

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

Recommended Mitigation Steps

Consider adding explicit zero-address validation on input parameters of address type.

[LOW‑11] Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

Proof of Concept

File: 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/tree/main/src/Beedle.sol#L5-L7

Recommended Mitigation Steps

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts. See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts

[LOW‑12] Solidity version 0.8.20 may not work on other chains due to PUSH0

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code may not yet be implemented on all L2s, so deployment on these chains will fail. 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.

Proof Of Concept

```solidity File: Beedle.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Beedle.sol#L2 ```solidity File: Fees.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Fees.sol#L2 ```solidity File: Lender.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Lender.sol#L2 ```solidity File: Staking.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/Staking.sol#L2 ```solidity File: IERC20.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/interfaces/IERC20.sol#L2 ```solidity File: ISwapRouter.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/interfaces/ISwapRouter.sol#L2 ```solidity File: Errors.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/utils/Errors.sol#L2 ```solidity File: Ownable.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/utils/Ownable.sol#L2 ```solidity File: Structs.sol pragma solidity ^0.8.19; ``` https://github.com/Cyfrin/2023-07-beedle/tree/main/src/utils/Structs.sol#L2

[LOW‑13] TransferOwnership Should Be Two Step

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Proof Of Concept

File: Ownable.sol

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

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

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

[LOW‑14] Unbounded loop

New items are pushed into the following arrays but there is no option to pop them out. Currently, the array can grow indefinitely. E.g. there's no maximum limit and there's no functionality to remove array values. If the array grows too large, calling relevant functions might run out of gas and revert. Calling these functions could result in a DOS condition.

Proof Of Concept

File: Lender.sol

276: loans.push(loan);

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

Recommended Mitigation Steps

Add a functionality to delete array values or add a maximum size limit for arrays.

[LOW‑15] 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.

Proof Of Concept

File: Beedle.sol

4: import {Ownable} from "./utils/Ownable.sol";
9: contract Beedle is Ownable, ERC20, ERC20Permit, ERC20Votes {
11: constructor() ERC20("Beedle", "BDL") ERC20Permit("Beedle") Ownable(msg.sender) {

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

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

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

File: Lender.sol

8: import {Ownable} from "./utils/Ownable.sol";
10: contract Lender is Ownable {
73: constructor() Ownable(msg.sender) {

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

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

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

File: Staking.sol

5: import {Ownable} from "./utils/Ownable.sol";
11: contract Staking is Ownable {
31: constructor(address _token, address _weth) Ownable(msg.sender) {

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

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

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

[LOW‑17] A year is not always 365 days

On leap years, the number of days is 366, so calculations during those years will return the wrong value

Proof Of Concept

File: Lender.sol

724: interest = (l.interestRate * l.debt * timeElapsed) / 10000 / 365 days;

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

PatrickAlphaC commented 1 year ago
  1. Is info
  2. is info
  3. does not result in dos
  4. need proof of issue
  5. is known
  6. This is not upgradebale
  7. doesnt dos
  8. is known
  9. is known