From 14a704dc6c9c190803e26058d6224c3a307b0eb7 Mon Sep 17 00:00:00 2001 From: sherlock-admin3 <162439078+sherlock-admin3@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:07:35 +0100 Subject: [PATCH] Uploaded files for judging --- .gitignore | 10 --- 001.md | 64 +++++++++++++ 002.md | 138 ++++++++++++++++++++++++++++ 003.md | 46 ++++++++++ 004.md | 34 +++++++ 005.md | 78 ++++++++++++++++ 006.md | 40 +++++++++ 007.md | 127 ++++++++++++++++++++++++++ 008.md | 149 ++++++++++++++++++++++++++++++ 009.md | 101 +++++++++++++++++++++ 010.md | 46 ++++++++++ 011.md | 28 ++++++ 012.md | 49 ++++++++++ 013.md | 77 ++++++++++++++++ 014.md | 138 ++++++++++++++++++++++++++++ 015.md | 41 +++++++++ 016.md | 64 +++++++++++++ 017.md | 110 +++++++++++++++++++++++ 018.md | 94 +++++++++++++++++++ 019.md | 52 +++++++++++ 020.md | 105 ++++++++++++++++++++++ 021.md | 79 ++++++++++++++++ 022.md | 78 ++++++++++++++++ 023.md | 144 +++++++++++++++++++++++++++++ 024.md | 80 +++++++++++++++++ 025.md | 41 +++++++++ 026.md | 42 +++++++++ 027.md | 65 ++++++++++++++ 028.md | 48 ++++++++++ 029.md | 133 +++++++++++++++++++++++++++ 030.md | 63 +++++++++++++ 031.md | 103 +++++++++++++++++++++ 032.md | 98 ++++++++++++++++++++ 033.md | 71 +++++++++++++++ 034.md | 100 +++++++++++++++++++++ 035.md | 85 ++++++++++++++++++ 036.md | 39 ++++++++ 037.md | 57 ++++++++++++ 038.md | 73 +++++++++++++++ 039.md | 77 ++++++++++++++++ 040.md | 50 +++++++++++ 041.md | 66 ++++++++++++++ 042.md | 85 ++++++++++++++++++ 043.md | 108 ++++++++++++++++++++++ 044.md | 41 +++++++++ 045.md | 51 +++++++++++ 046.md | 81 +++++++++++++++++ 047.md | 89 ++++++++++++++++++ 048.md | 206 ++++++++++++++++++++++++++++++++++++++++++ 049.md | 94 +++++++++++++++++++ 050.md | 72 +++++++++++++++ 051.md | 72 +++++++++++++++ 052.md | 60 +++++++++++++ 053.md | 229 +++++++++++++++++++++++++++++++++++++++++++++++ 054.md | 71 +++++++++++++++ 055.md | 51 +++++++++++ 056.md | 73 +++++++++++++++ 057.md | 75 ++++++++++++++++ 058.md | 58 ++++++++++++ 059.md | 101 +++++++++++++++++++++ 060.md | 81 +++++++++++++++++ 061.md | 90 +++++++++++++++++++ 062.md | 88 ++++++++++++++++++ 063.md | 195 ++++++++++++++++++++++++++++++++++++++++ 064.md | 44 +++++++++ 065.md | 131 +++++++++++++++++++++++++++ 066.md | 52 +++++++++++ 067.md | 136 ++++++++++++++++++++++++++++ 068.md | 41 +++++++++ 069.md | 47 ++++++++++ 070.md | 97 ++++++++++++++++++++ 071.md | 72 +++++++++++++++ 072.md | 87 ++++++++++++++++++ 073.md | 179 ++++++++++++++++++++++++++++++++++++ invalid/.gitkeep | 0 75 files changed, 6130 insertions(+), 10 deletions(-) delete mode 100644 .gitignore create mode 100644 001.md create mode 100644 002.md create mode 100644 003.md create mode 100644 004.md create mode 100644 005.md create mode 100644 006.md create mode 100644 007.md create mode 100644 008.md create mode 100644 009.md create mode 100644 010.md create mode 100644 011.md create mode 100644 012.md create mode 100644 013.md create mode 100644 014.md create mode 100644 015.md create mode 100644 016.md create mode 100644 017.md create mode 100644 018.md create mode 100644 019.md create mode 100644 020.md create mode 100644 021.md create mode 100644 022.md create mode 100644 023.md create mode 100644 024.md create mode 100644 025.md create mode 100644 026.md create mode 100644 027.md create mode 100644 028.md create mode 100644 029.md create mode 100644 030.md create mode 100644 031.md create mode 100644 032.md create mode 100644 033.md create mode 100644 034.md create mode 100644 035.md create mode 100644 036.md create mode 100644 037.md create mode 100644 038.md create mode 100644 039.md create mode 100644 040.md create mode 100644 041.md create mode 100644 042.md create mode 100644 043.md create mode 100644 044.md create mode 100644 045.md create mode 100644 046.md create mode 100644 047.md create mode 100644 048.md create mode 100644 049.md create mode 100644 050.md create mode 100644 051.md create mode 100644 052.md create mode 100644 053.md create mode 100644 054.md create mode 100644 055.md create mode 100644 056.md create mode 100644 057.md create mode 100644 058.md create mode 100644 059.md create mode 100644 060.md create mode 100644 061.md create mode 100644 062.md create mode 100644 063.md create mode 100644 064.md create mode 100644 065.md create mode 100644 066.md create mode 100644 067.md create mode 100644 068.md create mode 100644 069.md create mode 100644 070.md create mode 100644 071.md create mode 100644 072.md create mode 100644 073.md create mode 100644 invalid/.gitkeep diff --git a/.gitignore b/.gitignore deleted file mode 100644 index 3fbffbb..0000000 --- a/.gitignore +++ /dev/null @@ -1,10 +0,0 @@ -* -!*/ -!/.data -!/.github -!/.gitignore -!/README.md -!/comments.csv -!*.md -!**/*.md -!/Audit_Report.pdf diff --git a/001.md b/001.md new file mode 100644 index 0000000..177f5de --- /dev/null +++ b/001.md @@ -0,0 +1,64 @@ +Stable Graphite Rooster + +High + +# Function `_getStakeholdersForMarket` will be revert with stack overflow and `getAllVerifiedLendersForMarket` and `getAllVerifiedBorrowersForMarket` will revert. + +### Summary + +There is stack overflow in function `_getStakeholdersForMarket`. + +### Root Cause + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L1019 + +### Internal pre-conditions + +1. Anyone needs to call `getAllVerifiedLendersForMarket` or `getAllVerifiedBorrowersForMarket`. +2. In function `_getStakeholdersForMarket`, the conditions `start <= len`, `start < end` and `start > 0` will hold. + + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Someone calls `getAllVerifiedLendersForMarket` or `getAllVerifiedBorrowersForMarket`. +2. If `start` <= `len`, `start` < `end` and `start` > `0`, a stack overflow occurs at the line `stakeholders_[i] = _set.at(i);` + +### Impact + +The protocol fails to retrieve verified borrowers or lenders for market because it reverts due to a stack overflow. + +### PoC + +_No response_ + +### Mitigation + +The `_getStakeholdersForMarket` function should be corrected as follows. +```diff +function _getStakeholdersForMarket( + EnumerableSet.AddressSet storage _set, + uint256 _page, + uint256 _perPage + ) internal view returns (address[] memory stakeholders_) { + uint256 len = _set.length(); + + uint256 start = _page * _perPage; + if (start <= len) { + uint256 end = start + _perPage; + // Ensure we do not go out of bounds + if (end > len) { + end = len; + } + + stakeholders_ = new address[](end - start); + for (uint256 i = start; i < end; i++) { +- stakeholders_[i] = _set.at(i); ++ stakeholders_[i - start] = _set.at(i); // Correct indexing by offset + } + } + } +``` \ No newline at end of file diff --git a/002.md b/002.md new file mode 100644 index 0000000..e1d0211 --- /dev/null +++ b/002.md @@ -0,0 +1,138 @@ +Proper Daffodil Toad + +High + +# Inconsistent attestation expiration in `attestLender` and `attestBorrower` functions result in unauthorized actions or access in the market + +### Summary + +The contract suffers from an inconsistent attestation expiration issue, leading to the possibility of stakeholders (lenders and borrowers) being incorrectly marked as "attested" beyond their actual expiration time. This could result in unauthorized actions or access in the market, including attestation and participation in a market after expiration. + +### Root Cause + +The vulnerability lies within the `attestLender` and `attestBorrower` functions. When a lender or borrower is attested, they are granted certain permissions within the market, such as the ability to participate. However, the expiration time is not enforced consistently, which allows these stakeholders to continue having the permissions even after their attestation has expired. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L289-L295 +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L367-L373 +```solidity +function attestLender( + uint256 _marketId, + address _lenderAddress, + uint256 _expirationTime +) external { + _attestStakeholder(_marketId, _lenderAddress, _expirationTime, true); +} + +function attestBorrower( + uint256 _marketId, + address _borrowerAddress, + uint256 _expirationTime +) external { + _attestStakeholder(_marketId, _borrowerAddress, _expirationTime, false); +} + +function _attestStakeholder( + uint256 _marketId, + address _stakeholderAddress, + uint256 _expirationTime, + bool _isLender +) internal { + // Attestation logic + if (_expirationTime > block.timestamp) { + // Attestation is valid + if (_isLender) { + // Attest lender + markets[_marketId].verifiedLendersForMarket.add(_stakeholderAddress); + } else { + // Attest borrower + markets[_marketId].verifiedBorrowersForMarket.add(_stakeholderAddress); + } + } + // There should be an expiration check, but it's not present +} +``` +In the above code, while the expiration time is passed as a parameter, there is no proper expiration check against the current time `(block.timestamp)`. This means that even if the expiration time has passed, stakeholders remain attested without being removed from the market. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Attest a lender or borrower with an expiration time. +2. Try to perform actions related to the attestation after the expiration time has passed. +3. Verify that the stakeholder can still perform actions after their attestation has expired. + +### Impact + +This vulnerability allows attested stakeholders (lenders and borrowers) to continue acting in the market after their attestation has expired. The implications include: +1. Expired lenders or borrowers could still participate in a market and perform actions like making bids or entering agreements. +2. A lender who is no longer authorized might still provide funding, or a borrower could enter a loan agreement after the expiration of their attestation. +3. Since market behaviors and transactions may depend on validated actors (lenders and borrowers), expired attestations could disrupt the integrity of the market or lead to unauthorized access. + +### PoC + +```solidity +pragma solidity ^0.8.0; + +import "ds-test/test.sol"; // Use for testing +import { MarketRegistry } from "./MarketRegistry.sol"; // The contract we are testing + +contract TestAttestationExpiration is DSTest { + MarketRegistry marketRegistry; + + uint256 marketId; + address lender = address(0x123); + address borrower = address(0x456); + uint256 expirationTime; + + function setUp() public { + marketRegistry = new MarketRegistry(); + marketId = 1; // Example market ID + expirationTime = block.timestamp + 1 hours; // Set expiration time to 1 hour from now + } + + function testAttestationExpiration() public { + // Attest a lender with expiration time + marketRegistry.attestLender(marketId, lender, expirationTime); + // Wait for 2 hours to simulate expiration + vm.warp(block.timestamp + 2 hours); + + // Attempt to perform an action after expiration + bool canLenderExit = marketRegistry.lenderExitMarket(marketId); + + // Expectation: The lender should not be able to exit the market after attestation expires + assertEq(canLenderExit, false, "Lender should not be able to exit after expiration"); + } +} +``` +The test is expected to fail because the lender is able to exit the market despite the expiration time having passed. This shows that the expiration check is missing or improperly handled. +```bash +Fail: Lender should not be able to exit after expiration +``` + + +### Mitigation + +To fix the issue, the code should enforce proper expiration checks to remove lenders or borrowers after their attestation expires. The `_attestStakeholder` function should be modified to reject actions when the attestation has expired. +```solidity +function _attestStakeholder( + uint256 _marketId, + address _stakeholderAddress, + uint256 _expirationTime, + bool _isLender +) internal { + // Ensure expiration time is greater than the current block timestamp + require(_expirationTime > block.timestamp, "Attestation has expired"); + + // Proceed with attestation as usual + if (_isLender) { + markets[_marketId].verifiedLendersForMarket.add(_stakeholderAddress); + } else { + markets[_marketId].verifiedBorrowersForMarket.add(_stakeholderAddress); + } +} +``` \ No newline at end of file diff --git a/003.md b/003.md new file mode 100644 index 0000000..ab4edf1 --- /dev/null +++ b/003.md @@ -0,0 +1,46 @@ +Scrawny Linen Snail + +High + +# If repayLoanCallback address doesn't implement repayLoanCallback try/catch won't go into the catch and will revert the tx + +### Summary + +If `repaymentListenerForBid` contract doesn't implement the `repayLoanCallback`, repaying of loan transaction will revert. + +### Root Cause + +If a contract, which is set as `loanRepaymentListener` from a lender doesn't implement `repayLoanCallback` transaction will revert and catch block won't help. + +This is serious and even crucial problem, because a malicous lender could prevent borrowers from repaying their loans, as repayLoanCallback is called inside `_repayLoan `. This way he guarantees himself their collateral tokens. + +[Converastion explaining why try/catch helps only if transaction is reverted in the target, contrac, which is not the case here](https://ethereum.stackexchange.com/questions/129150/solidity-try-catch-call-to-external-non-existent-address-method) + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L828-L876 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Lenders can stop borrowers from repaying their loans, forcing their loans to default and revert. + +### PoC + +Let's have the following scenario: + +1. Lender sets the repaymentListenerForBid[_bidId] to a contract which doesn't have `repayLoanCallback` function. +2. Borrower can't repay their debt. + +### Mitigation + +This bug was found in the previous audit. Consider using the same fix as the previous time: https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/31 \ No newline at end of file diff --git a/004.md b/004.md new file mode 100644 index 0000000..d74d761 --- /dev/null +++ b/004.md @@ -0,0 +1,34 @@ +Dazzling Metal Hippo + +Medium + +# Incorrect Handling of Desired Overflow in `FullMath` Library with Solidity ≥0.8 + +## Summary + +The `FullMath` library, originally designed for Solidity `<0.8.0` in `Uniswap V3`, has been modified to work with Solidity `>=0.8.0` in the Teller protocol. However, this modification does not handle the library's reliance on "phantom overflow" effectively. As a result, the library fails when intermediate calculations exceed the 256-bit limit, causing execution to revert. + +## Vulnerability Details + +The Uniswap V3's [FullMath.sol](https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol) library was originally designed to allow intermediate overflows during multiplication and division operations, referred to as "phantom overflow." The library was designed for Solidity `<0.8.0`, relying on the absence of built-in overflow checks. + +However, the version of the [FullMath.sol](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/uniswap/FullMath.sol#L2) library ported to Teller protocol has been compiled with Solidity `>=0.8.0`, which introduces built-in overflow/underflow protection. + +The library is used in Teller’s `UniswapPricingLibrary.sol` and plays a critical role in price calculations. Since Solidity `>=0.8.0` automatically reverts on arithmetic overflow, the affected operations within the `FullMath` library fail when intermediate values exceed 256 bits. + +As a result, multiplication and division operations that rely on the expected intermediate overflows now revert, breaking the intended functionality of the library. + + +Although `FullMath` itself is an out-of-scope library, its use in an in-scope contract (`UniswapPricingLibrary.sol`) makes this issue valid under the following **Sherlock** rule: +> "In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug, this is a valid issue." + +**Similar Past Issue on Sherlock**: +[UXD_01_2023](https://github.com/sherlock-audit/2023-01-uxd-judging/issues/273) + + +## Impact +The correct result isn't returned in this case and the execution gets reverted when a phantom overflows occurs. + +## Recommendation +The affected `FullMath` library should be updated to the latest version that explicitly handles overflows using `unchecked` blocks. + Modified version of FullMath designed for Solidity `>=0.8.0` is available in the [Uniswap v3-core repository (0.8 branch)](https://github.com/Uniswap/v3-core/blob/0.8/contracts/libraries/FullMath.sol). diff --git a/005.md b/005.md new file mode 100644 index 0000000..ca4e516 --- /dev/null +++ b/005.md @@ -0,0 +1,78 @@ +Scrawny Linen Snail + +Medium + +# Anyone can remove lender and borrowers from MarketRegistry + +### Summary + +In the current implementation of the `MarketRegistry` contract, any actor can revoke a lender or borrower for markets that require lender or borrower attestation. + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L332 + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L1192-L1208 + +### Root Cause + +The contract defines two versions of the `revokeLender` function: + +1. **Function `revokeLender(uint256 _marketId, address _lenderAddress, uint8 _v, bytes32 _r, bytes32 _s)`** + This function internally calls `_revokeStakeholderViaDelegation` but does not verify the signature (`v, r, s`) against the market owner, nor does it enforce any other ownership or permission checks. This omission allows unauthorized users to revoke stakeholders. + + Example code from `_revokeStakeholderViaDelegation`: + ```solidity + function _revokeStakeholderViaDelegation( + uint256 _marketId, + address _stakeholderAddress, + bool _isLender, + uint8 _v, + bytes32 _r, + bytes32 _s + ) internal { + bytes32 uuid = _revokeStakeholderVerification( + _marketId, + _stakeholderAddress, + _isLender + ); + // Note: Call to revoke attestation on EAS contracts is disabled. + } + ``` + +2. **Function `revokeLender(uint256 _marketId, address _lenderAddress)`** + This version includes a check to ensure the caller is the market owner. + +The discrepancy between these two versions creates an exploitable vulnerability. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Unauthorized revocation of lenders or borrowers can disrupt the proper functioning of markets that rely on stakeholder attestations. This could lead to: + +- Denial of service for legitimate lenders and borrowers. +- Loss of trust in the platform. +- Potential financial losses for affected parties. + +### PoC + +This vulnerability can be exploited by crafting a transaction to call revokeLender or revokeBorrower with arbitrary v, r, s values. + +```solidity +revokeLender(marketId, victimAddress, randomV, randomR, randomS); +``` + +This call bypasses ownership and attestation verification, removing the specified lender. + +### Mitigation + +**Add Verification** : Update the revokeLender and revokeBorrower functions that accept v, r, s to enforce attestation verification against the market owner's signature \ No newline at end of file diff --git a/006.md b/006.md new file mode 100644 index 0000000..c303cf5 --- /dev/null +++ b/006.md @@ -0,0 +1,40 @@ +Round Neon Kestrel + +Medium + +# Reentrancy attack in collateral commitment + +### Summary + +The lack of reentrancy protection in the `commitCollateral()` function will cause a vulnerability for the protocol as an attacker can re-enter the function, causing unintended behavior and potentially draining the collateral. + +### Root Cause + +In `CollateralManager.sol:88`, the function `commitCollateral()` involves a state-changing operation (mapping update) and a token transfer without any reentrancy protection. This could lead to a reentrancy attack where an attacker can recursively call `commitCollateral()` and manipulate the state during the process. + +### Internal pre-conditions + +1. Attacker needs to invoke `commitCollateral()` while holding control over the execution flow, such as through an external call to a vulnerable contract. + +2. The attacker needs to ensure the collateral is of a valid type and has sufficient amount to be deposited. + +### External pre-conditions + +Attacker needs to control a malicious contract that can re-enter `commitCollateral()` during execution. + +### Attack Path + +1. The attacker creates a malicious contract that calls `commitCollateral()`. +2. During execution, the malicious contract re-enters the function, manipulating collateral or bypassing checks. + +### Impact + +The protocol suffers potential collateral loss, as attackers can repeatedly deposit collateral without it being properly accounted for, draining assets from the protocol. The attacker gains the ability to manipulate collateral deposits, causing financial loss. + +### PoC + +_No response_ + +### Mitigation + +Use a `ReentrancyGuard` modifier or implement the checks-effects-interactions pattern to prevent reentrancy issues. \ No newline at end of file diff --git a/007.md b/007.md new file mode 100644 index 0000000..efabb2c --- /dev/null +++ b/007.md @@ -0,0 +1,127 @@ +Faint Flaxen Pangolin + +Medium + +# `TellerV2.sol` inherits from `ProtocolFee.sol` contract that lacks storage gaps in a complex inconsistent inheritance chain, that will cause storage corruption in future contract upgrades. + +### Summary + +`TellerV2.sol`'s parent contracts implement storage gaps inconsistently across its inheritance chain. While some contracts like `HasProtocolPausingManager` properly implement storage gaps, `ProtocolFee` initializes upgradeability but lacks storage gaps which it **NEEDS**. This inconsistency in a complex, branched inheritance structure creates risks for future upgrades. + +This vulnerablility finding aligns with Sherlock's validity rule which state: + +_**"if the protocol design has a highly complex and branched set of contract inheritance with storage gaps inconsistently applied throughout and the submission clearly describes the necessity of storage gaps it can be considered a valid medium."**_ + +### Root Cause + +The `TellerV2` contract has a complex inheritance structure where storage gaps are implemented inconsistently: + +```mermaid +graph BT; + classDef nogap fill:#fc0345; + classDef hasgap fill:#0384fc; + TellerV2:::nogap-->HasProtocolPausingManager:::hasgap + TellerV2:::nogap-->ProtocolFee:::nogap + ProtocolFee:::nogap-->OwnableUpgradeable:::hasgap +``` + +1. Contract inheritance structure: + +```solidity +contract TellerV2 is + ITellerV2, + ILoanRepaymentCallbacks, + OwnableUpgradeable, // Has storage gaps + ProtocolFee, // Missing storage gaps despite upgradeability + HasProtocolPausingManager, // Has storage gaps + TellerV2Storage, + TellerV2Context +``` +2. ProtocolFee.sol initializes upgradeability but lacks storage gaps: + +```solidity +contract ProtocolFee is OwnableUpgradeable { + uint16 private _protocolFee; + + function __ProtocolFee_init(uint16 initFee) internal onlyInitializing { + __Ownable_init(); // Initializes upgradeability + __ProtocolFee_init_unchained(initFee); + } + // No storage gaps implemented +} +``` +3. In contrast, HasProtocolPausingManager.sol properly implements storage gaps: + +```solidity +abstract contract HasProtocolPausingManager is IHasProtocolPausingManager { + bool private __paused; + address private _protocolPausingManager; + + // Properly implements storage gaps + uint256[49] private __gap; +} +``` +This inconsistency is particularly concerning because: + +1. The inheritance structure is branched rather than linear, increasing complexity. +2. ProtocolFee is a concrete contract that's actively used in the inheritance chain. +3. The codebase shows awareness of proper upgradeability patterns (gaps in HasProtocolPausingManager) but fails to implement them consistently. + +See: [https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps) + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +Not Necessary + +### Impact + +The lack of storage gaps in ProtocolFee while implementing upgradeability creates a risk of storage collision during future upgrades. This is particularly problematic because: + +- ProtocolFee is a concrete contract in active use +- It's part of a complex inheritance chain with multiple branches +- Other contracts in the same inheritance tree implement storage gaps, showing inconsistency in upgradeability pattern implementation + +### PoC + +Please refer to [TellerV2.sol](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L49-L56) and ProtocolFee.sol + +### Mitigation + +Add storage gaps to ProtocolFee.sol to maintain consistency with other upgradeable contracts in the inheritance chain + +```solidity +contract ProtocolFee is OwnableUpgradeable { + uint16 private _protocolFee; + + function __ProtocolFee_init(uint16 initFee) internal onlyInitializing { + __Ownable_init(); + __ProtocolFee_init_unchained(initFee); + } + + // Add storage gaps + uint256[49] private __gap; +} +``` + +This ensures all contracts in the inheritance chain consistently implement proper upgradeability patterns, reducing the risk of storage collisions in future upgrades. + +Additionally, in ProtocolFee.sol and TellerV2.sol as a security best practice implement: + +```solidity +constructor() { + _disableInitializers(); +} +``` +This ensures: + +1. The implementation contract can never be initialized directly. +2. All initialization must go through the proxy. +3. Prevents potential manipulation of the implementation's state. \ No newline at end of file diff --git a/008.md b/008.md new file mode 100644 index 0000000..0918595 --- /dev/null +++ b/008.md @@ -0,0 +1,149 @@ +Original Lace Sheep + +High + +# Price manipulation in LenderCommitmentGroup_Smart lending pools remains a critical vulnerability despite acknowledged risk + +### Summary + +Despite protocol's acknowledged risk, the use of Uniswap's current slot0 price (when twapInterval = 0) presents an unacceptable vulnerability that can lead to large scale losses and is just an overall unacceptable risk to have within the Teller protocol. Economic assumptions about pool size ratios fail to protect against sophisticated MEV and flash loan attacks, making this an urgent security concern requiring immediate mitigation. + +Critical Call Flow: +```solidity +acceptFundsForAcceptBid +└── calculateCollateralRequiredToBorrowPrincipal + └── calculateCollateralTokensAmountEquivalentToPrincipalTokens + └── UniswapPricingLibrary.getUniswapPriceRatioForPoolRoutes + └── getSqrtTwapX96 (vulnerable point) +``` + +### Root Cause + +In `UniswapPricingLibrary.sol:60` the `getSqrtTwapX96()` function allows `twapInterval = 0` which defaults to using current Uniswap slot0 price. While acknowledged, real-world attack scenarios demonstrate this "accepted risk" is based on flawed economic assumptions: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/UniswapPricingLibrary.sol#L103 + +```solidity +function getSqrtTwapX96(address uniswapV3Pool, uint32 twapInterval) internal view { + if (twapInterval == 0) { + // return the current price if twapInterval == 0 + (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(uniswapV3Pool).slot0(); + } +} +``` +Affected Contracts and Functions: +1. **UniswapPricingLibrary.sol** +- `getSqrtTwapX96()` - Core vulnerability point +- `getUniswapPriceRatioForPoolRoutes()` +- `getUniswapPriceRatioForPool()` + +2. **LenderCommitmentGroup_Smart.sol** +- `acceptFundsForAcceptBid()` +- `calculateCollateralRequiredToBorrowPrincipal()` +- `calculateCollateralTokensAmountEquivalentToPrincipalTokens()` +- `getUniswapPriceRatioForPoolRoutes()` +- `getPrincipalForCollateralForPoolRoutes()` +- `initialize()` + +3. **LenderCommitmentGroupFactory.sol** +- `deployLenderCommitmentGroupPool()` +### Proof of Flawed Risk Assessment + +Example Profitable Attack (Even Within Protocol's Safety Parameters): +1. LenderCommitmentGroup_Smart pool: $1M lending capacity +2. Uniswap pool: $10M TVL (adhering to protocol's "10x larger" safety assumption) +3. Attack using $5M flash loan (0.09% fee = $4,500) +4. Price manipulation potential: + - 30-40% temporary price movement achievable + - Original collateral requirement: $1M collateral → $666K borrowing + - Manipulated price (1.4x): Same $1M collateral → $933K borrowing +5. Profit Calculation: + ```solidity + Costs: + - Flash loan fee: $4,500 + - Gas + MEV: ~$500 + - Slippage (~1%): $50,000 + Total costs: ~$55,000 + + Revenue: + - Borrowed amount: $933K + - True collateral value: $666K + Net profit: $212,000 per attack + ``` + +### Internal pre-conditions + +1. A LenderCommitmentGroup_Smart pool must be deployed with `twapInterval = 0` in its oracle configuration +2. The pool must have sufficient principal tokens available (determined by `getPrincipalAmountAvailableToBorrow()`) +3. The pool must be unpaused and active (`whenNotPaused` modifier satisfied) +4. The collateral ratio must be set (typically >100% via `collateralRatio`) + + +### External pre-conditions + +1. Sufficient liquidity in relevant Uniswap V3 pools to execute flash loans and achieve meaningful price manipulation +2. The targeted token pair must have enough price impact potential relative to pool depth to make the attack profitable after gas costs +3. No external circuit breakers or price deviation checks that would prevent the price manipulatio + +### Attack Path + +1. Attacker identifies a LenderCommitmentGroup_Smart pool using vulnerable price feeds +2. Attacker takes a flash loan of collateral token from any lending protocol +3. Attacker executes a large swap in the Uniswap pool to manipulate the slot0 price, artificially increasing collateral token value +4. Attacker calls `acceptFundsForAcceptBid()` with manipulated prices allowing them to: + ```solidity + function acceptFundsForAcceptBid( + address _borrower, + uint256 _bidId, + uint256 _principalAmount, + uint256 _collateralAmount, + ... + ) external { + uint256 requiredCollateral = calculateCollateralRequiredToBorrowPrincipal(_principalAmount); + require(_collateralAmount >= requiredCollateral, "Insufficient Borrower Collateral"); + ... + } + ``` + - Provide less collateral than normally required due to manipulated price ratio + - Borrow maximum principal amount based on inflated collateral value +5. Attacker repays flash loan, allowing price to return to normal +6. Loan remains active but severely under-collateralized based on true market price + +### Impact + +This vulnerability must be addressed because: +1. Protocol's economic assumptions fail: + - Flash loans eliminate capital requirements + - MEV strategies can optimize profit beyond basic calculations + - Real profit potential exists even within "safe" parameters + - $200K+ profit per attack proves economic deterrence insufficient + +2. Systemic risks: + - Multiple pools can be attacked sequentially + - MEV bots can automate and optimize attacks + - No technical barriers prevent exploitation + +3. Market Impact: + - Protocol reputation damage + - Lender confidence erosion + - Potential cascade effect across integrated protocols + + +### PoC + +_No response_ + +### Mitigation + +1. Remove `twapInterval = 0` option entirely +2. Implement mandatory minimum TWAP period (suggest 30 minutes) +3. Add price deviation circuit breakers +4. Consider multiple oracle sources for validation + +The protocol's current stance of "make it unprofitable" is demonstrably insufficient given: +- Proven profit potential even within safety parameters +- Evolving MEV strategies +- Flash loan accessibility +- Automation potential + +This issue requires immediate attention as economic deterrence has been proven inadequate for modern DeFi security. \ No newline at end of file diff --git a/009.md b/009.md new file mode 100644 index 0000000..f1c3d2e --- /dev/null +++ b/009.md @@ -0,0 +1,101 @@ +Original Lace Sheep + +Medium + +# Missing OpenZeppelin Upgradeable Initializations will Impact Protocol Security and Functionality + +### Summary + +Multiple contracts failing to initialize inherited OpenZeppelin upgradeable contracts will cause a systemic issue for protocol users as uninitialized contracts may operate with improper state variables and access controls. + +### Root Cause + +In the following contracts, there are missing initializations of inherited OpenZeppelin upgradeable contracts: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L81-L88 + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L49-L52 + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/SmartCommitmentForwarder.sol#L51-L55 + + + +1. In `LenderCommitmentGroup_Smart.sol`, the contract inherits ReentrancyGuardUpgradeable but fails to call `__ReentrancyGuard_init()` +2. In `TellerV2.sol`, the contract inherits OwnableUpgradeable but fails to call `__Ownable_init()` +3. In `SmartCommitmentForwarder.sol`, the contract inherits ReentrancyGuardUpgradeable but fails to call `__ReentrancyGuard_init()` + + +### Internal pre-conditions + +1. Contracts must be deployed and initialized through their respective proxy patterns +2. For LenderCommitmentGroup_Smart and SmartCommitmentForwarder, reentrancy protection would need to be required for a function execution +3. For TellerV2, ownership functionality would need to be invoked + +### External pre-conditions + +No external pre-conditions required as this is an implementation issue. + +### Attack Path + +For TellerV2: +1. Contract is deployed and initialized without proper `__Ownable_init()` +2. Initial owner state may be improperly set +3. Ownership functions could behave unexpectedly or fail +4. Critical owner-only functions may become inaccessible or improperly secured + +For LenderCommitmentGroup_Smart and SmartCommitmentForwarder: +1. Contracts are deployed without proper `__ReentrancyGuard_init()` +2. Reentrancy guard state variables are not properly initialized +3. nonReentrant modifier may not provide expected protection +4. Potential for reentrancy attacks on protected functions + +### Impact + +The affected parties (protocol and users) face multiple risks: + +1. LenderCommitmentGroup_Smart: +- Missing reentrancy protection could allow unauthorized reentrant calls +- Critical functions marked with nonReentrant may be vulnerable +- Potential loss of funds through reentrancy attacks + +2. TellerV2: +- Ownership functionality may be compromised +- Administrative functions could become inaccessible +- Security of owner-protected functions may be compromised + +3. SmartCommitmentForwarder: +- Similar to LenderCommitmentGroup_Smart, reentrancy attacks possible +- Protected functions may be vulnerable to exploitation +- Potential loss of funds or unauthorized operations + +The overall protocol security could be significantly compromised, potentially leading to unauthorized access, fund loss, or contract functionality failure. + +### PoC + +_No response_ + +### Mitigation + +1. Add proper initialization calls in the affected contracts' `initialize()` functions: + +```solidity +// LenderCommitmentGroup_Smart.sol +function initialize(...) initializer { + __ReentrancyGuard_init(); + // Rest of initialization logic +} + +// TellerV2.sol +function initialize(...) initializer { + __Ownable_init(); + // Rest of initialization logic +} + +// SmartCommitmentForwarder.sol +function initialize(...) initializer { + __ReentrancyGuard_init(); + // Rest of initialization logic +} +``` + +2. Ensure the `initializer` modifier is used to prevent multiple initializations \ No newline at end of file diff --git a/010.md b/010.md new file mode 100644 index 0000000..cacab16 --- /dev/null +++ b/010.md @@ -0,0 +1,46 @@ +Blunt Alabaster Kookaburra + +Medium + +# Incompatibility of FullMath Library with Solidity's Overflow Protection + +### Summary + +The `FullMath` library, adapted for Teller's protocol from Uniswap V3, fails to handle overflows correctly under Solidity ≥0.8.0. This causes execution to revert during critical calculations when intermediate values exceed the 256-bit limit, disrupting the intended functionality of the library. + + +### Root Cause + +The original `FullMath` library in UniswapV3 was designed for Solidity <0.8.0, where overflows were not automatically checked. +[https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol](https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol) +But in Teller protocol, the `FullMath` library has been adapted from Uniswap V3 for use with Solidity ≥0.8.0. +[https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/uniswap/FullMath.sol#L2](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/uniswap/FullMath.sol#L2) + +With Solidity ≥0.8.0, overflow checks are enforced, causing reversion in calculations that rely on the "phantom overflow" behavior of the original library. + +This effectively means that when a phantom overflow (a multiplication and division where an intermediate value overflows 256 bits) occurs the execution will revert and the correct result won't be returned. + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Execution reverts during calculations requiring intermediate overflows + +### PoC + +_No response_ + +### Mitigation + +Replace the current `FullMath` implementation with the 0.8 version in the Uniswap repository. diff --git a/011.md b/011.md new file mode 100644 index 0000000..1d55995 --- /dev/null +++ b/011.md @@ -0,0 +1,28 @@ +Fast Fiery Crane + +High + +# Lenders can't accept bids + +### Summary + +In the `submitBid`(with collateral) function of `TellerV2.sol`, approving process is missing. + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L306-L335 + +This makes impossible for lenders to accept bids, because the following code will be reverted. + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L405-L409 + + +### Root Cause + +In `TellerV2.sol:335`, there is a missing process to approve for `CollateralManager`. + +### Impact + +Lenders can't accept borrower's bids. + +### Mitigation + +Add approve process in `TellerV2.sol:335`. \ No newline at end of file diff --git a/012.md b/012.md new file mode 100644 index 0000000..8d21fd9 --- /dev/null +++ b/012.md @@ -0,0 +1,49 @@ +Dancing Cornflower Stallion + +Medium + +# Unsafe usage of approve method for some ERC20s + +### Summary + +The "default" ERC20 behavior expects the `approve` function to return a boolean. however, some ERC20s on some chains don't return a value (The most popular example is USDT on the Ethereum Mainnet). + +### Root Cause + +Since it is stated in readme that CHAINS and ERC20 tokens will be in use: + +> Ethereum Mainnet, Polygon PoS, Arbitrum One, Base + +> we already took special consideration to make sure USDT works with our contracts. + +So for non-standard token such as USDT, calling approve will revert because the solmate ERC20 enforce the underlying token return a boolean. + +### Internal pre-conditions + +Standard ERC20s return a boolean on approval: +https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L67 + +### External pre-conditions + +USDT on the main net doesn't return a value: +https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code + +### Attack Path + +DOS. + +### Impact + +`acceptFundsForAcceptBid` function will revert , if the principal token is USDT. +```solidity +principalToken.approve(address(TELLER_V2), _principalAmount); +``` + +### PoC + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L556 + +### Mitigation + +Use `forceApprove` instead of `approve`. + diff --git a/013.md b/013.md new file mode 100644 index 0000000..da50d00 --- /dev/null +++ b/013.md @@ -0,0 +1,77 @@ +Joyful Olive Meerkat + +Medium + +# Bid Submission Vulnerable to Changes in Market Parameters + +### Summary + +_No response_ + +### Root Cause + +In [_submitBid()](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L337), certain bid parameters are taken from the `marketRegistry`: + +```solidity + function _submitBid(...) + ... + (bid.terms.paymentCycle, bidPaymentCycleType[bidId]) = marketRegistry + .getPaymentCycle(_marketplaceId); + + bid.terms.APR = _APR; + + bidDefaultDuration[bidId] = marketRegistry.getPaymentDefaultDuration( + _marketplaceId + ); + + bidExpirationTime[bidId] = marketRegistry.getBidExpirationTime( + _marketplaceId + ); + + bid.paymentType = marketRegistry.getPaymentType(_marketplaceId); + + bid.terms.paymentCycleAmount = V2Calculations + .calculatePaymentCycleAmount( + bid.paymentType, + bidPaymentCycleType[bidId], + _principal, + _duration, + bid.terms.paymentCycle, + _APR + ); + ... +``` + +All the parameters taken from `marketRegistry` are controlled by the market owner: +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L503 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +If market parameters are changed in between the borrower submitting a bid transaction and the transaction being applied, borrower may be subject to changes in `bidDefaultDuration`, `bidExpirationTime`, `paymentType`, `paymentCycle`, `bidPaymentCycleType` and `paymentCycleAmount`. + +That is, the user may be committed to the bid for longer / shorter than expected. They may have a longer / shorter default duration (time for the loan being considered defaulted / eligible for liquidation). They have un-provisioned for payment type and cycle parameters. + +I believe most of this will have a medium impact on borrower (mild inconveniences / resolvable by directly repaying the loan) if the market owner is not evil and adapting the parameters reasonably. + +An evil market owner can set the value of `bidDefaultDuration` and `paymentCycle` very low (0) so that the loan will default immediately. It can then accept the bid, make user default immediately, and liquidate the loan to steal the user's collateral. This results in a loss of collateral for the borrower. + + +### PoC + +_No response_ + +### Mitigation + +Take every single parameters as input of `_submitBid()` (including fee percents) and compare them to the values in `marketRegistry` to make sure borrower agrees with them, revert if they differ. diff --git a/014.md b/014.md new file mode 100644 index 0000000..c6ea931 --- /dev/null +++ b/014.md @@ -0,0 +1,138 @@ +Zany Tweed Quail + +Medium + +# "Integer Overflow in `_valueOfUnderlying` Function of the `LenderCommitmentGroup_Smart` Contract" + +### Summary + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L494 + +The `_valueOfUnderlying` function calculates the share amount to be minted to the share recipient; however, the calculations within the function are prone to overflow. + + +### Root Cause + +The issue originates in the `_valueOfUnderlying` function, which calculates the share amount. However, improper handling of the calculation results in an integer overflow, causing it to reach the maximum value of `uint256`. and it affects the `addPrincipalToCommitmentGroup` + +` + + function _valueOfUnderlying(uint256 amount, uint256 rate) + internal + pure + returns (uint256 value_) + { + if (rate == 0) { + return 0; + } + + value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate; @audit + }` + + ` function addPrincipalToCommitmentGroup( + uint256 _amount, + address _sharesRecipient + ) external returns (uint256 sharesAmount_) { + //transfers the primary principal token from msg.sender into this contract escrow + + principalToken.transferFrom(msg.sender, address(this), _amount); + + sharesAmount_ = _valueOfUnderlying(_amount, sharesExchangeRate()); @audit It Revert here + + totalPrincipalTokensCommitted += _amount; + //principalTokensCommittedByLender[msg.sender] += _amount; + + //mint shares equal to _amount and give them to the shares recipient !!! + poolSharesToken.mint(_sharesRecipient, sharesAmount_); + }` + ` + + + +### Impact + +Integer Overflow on the `_valueOfUnderlying` function which leads to overflow on the calculation of the share amount and it affects the `addPrincipalToCommitmentGroup` function + +### PoC + +` ` // SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +contract Overflow { + uint256 public immutable EXCHANGE_RATE_EXPANSION_FACTOR = 1e36; + uint256 public totalSupply = 1e18; + uint256 public totalPrincipalTokensCommitted = 100e36; + uint256 public totalPrincipalTokensWithdrawn = 10e36; + + uint256 public totalInterestCollected = 50e36; + int256 public tokenDifferenceFromLiquidations = 20e36; + + function sharesExchangeRate() public view virtual returns (uint256 rate_) { + uint256 poolTotalEstimatedValue = getPoolTotalEstimatedValue(); + + if (totalSupply == 0) { + return EXCHANGE_RATE_EXPANSION_FACTOR; // 1 to 1 for first swap + } + + rate_ = (poolTotalEstimatedValue * EXCHANGE_RATE_EXPANSION_FACTOR) / totalSupply; + } + + function sharesExchangeRateInverse() public view virtual returns (uint256 rate_) { + return (EXCHANGE_RATE_EXPANSION_FACTOR * EXCHANGE_RATE_EXPANSION_FACTOR) / sharesExchangeRate(); + } + + function getPoolTotalEstimatedValue() public view returns (uint256 poolTotalEstimatedValue_) { + int256 poolTotalEstimatedValueSigned = int256(totalPrincipalTokensCommitted) + int256(totalInterestCollected) + + int256(tokenDifferenceFromLiquidations) - int256(totalPrincipalTokensWithdrawn); + + //if the poolTotalEstimatedValue_ is less than 0, we treat it as 0. + poolTotalEstimatedValue_ = + poolTotalEstimatedValueSigned > int256(0) ? uint256(poolTotalEstimatedValueSigned) : 0; + } + + + // Change the visibility to public for testing purpose + function _valueOfUnderlying(uint256 amount, uint256 rate) public pure returns (uint256 value_) { + if (rate == 0) { + return 0; + } + + value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate; + } + +} + ` + +// The contract + +` +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +import {Test, console} from "forge-std/Test.sol"; +import {Counter} from "../src/Counter.sol"; + +contract OverFlowTest is Test { + Overflow public overflow; + + function setUp() public { + overflow = new Overflow(); + } + + function testFuzz_valueofUnderlying(uint256 amount) public { + uint256 poolTotalEstimatedValue = overflow.getPoolTotalEstimatedValue(); + console.log(poolTotalEstimatedValue); + uint256 rate = counter.sharesExchangeRate(); + // console.log(rate); + uint256 value = counter._valueOfUnderlying(amount, rate); + + console.log(value); + } + + + } + ` ` + +### Mitigation + +Recheck the _valueOfUnderlying` function calculation to handle the shares amount calculation properly \ No newline at end of file diff --git a/015.md b/015.md new file mode 100644 index 0000000..d2d4a5b --- /dev/null +++ b/015.md @@ -0,0 +1,41 @@ +Merry Lead Orangutan + +High + +# MarketPlace Owner can change fee after the bid is already submited + +### Summary + +The way it currently works [`amountToMarketplace`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L517C1-L517C17) is calculated in [`lenderAcceptBid()` ](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L473) based on the current market fee which can be changed by the marketplace owner between the time a user submits a bid and the time the lender accepts it, resulting in loss of funds for the user that submitted the bid. + +### Root Cause + +In [`TellerV2.sol:517`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L517) the `amountToMarketplace` is calculated using the current market fee which can be changed at any time by the market owner and it allows a market owner using `updateMarketSettings()` in [`MarketRegistry.sol:503`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L503C14-L503C34) to trick the user into thinking the fee is very low and then change it after the bid is placed, if the market owner is also a lender it can increase the fee and accept the bid in the same transaction leaving the user without any chance of canceling the bid after the fee increase. The market owner can just as well have a secondary wallet or a deployed contract that is a lender and use that to hide his malicious intentions. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. User finds a market with a low fee to bid on +2. Marketplace owner changes the fee to max fee amount using [`updateMarketSettings()`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L503C14-L503C34) +3. Lender accepts the bid using [`lenderAcceptBid()`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L473) + +### Impact + +Users can lose funds (up to max market fee of 10%) to the market because the market fee can be modified before the lender accepts it + +### PoC + +_No response_ + +### Mitigation + +The market fee can be placed inside the bid structure and set once the user uses [`submitBid()`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L275) as to not allow the market owner to change it after the user submitted the bid + +Other less practical mitigation: set up a time delay between when the market owner can update the market settings \ No newline at end of file diff --git a/016.md b/016.md new file mode 100644 index 0000000..b0b98fa --- /dev/null +++ b/016.md @@ -0,0 +1,64 @@ +Restless Magenta Mallard + +Medium + +# Liquidation may not happen and can cause lock of collateral indefinitely. + +### Summary + +Liquidation can be triggered once the loan defaults, meaning the borrower fails to pay after loan duration expires. The liquidation will be triggered by the interested liquidators to acquire the collateral token assets in exchange of principal token loan payments. + +However, liquidators won't simply trigger the liquidation for the sake of "acquiring collateral assets", there should be profit in order to encourage the liquidators to do that. In the protocol pertaining to the loan bids under LenderCommitmentGroup_smart (LCG) contract, there is situation that it can't be profitable due significant deviation of USD price between collateral token and principal (loan) token. In this case, the principal (loan) token increase significantly compared to collateral token in terms of USD price. This can greatly affect the sentiment of liquidators and delay the liquidation. + + In other words, the liquidators won't pay for example 1 WETH principal (loan) token in exchange of 10,000 doge (collateral) coins if the current exchange rate increase significantly from 10,000 doge coins to 100,000 doge coins per 1 weth since the loan started. That's 10x more expensive. + +The lender LenderCommitmentGroup (LCG) [contract](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L82) may try to close the loan during this special situation in order for at least recover the collateral, just in case liquidation take too long but this contract has no capability to retrieve it. + +In one last desperate attempt, the developers may apply to upgrade the contracts affected in order to "retrieve the collateral", however i see this as a complex process and not easy to do specially if there so many loan bids affected. The contract or contracts to be upgraded may undergo several iterations, tests, audits before it will deployed again securely live on-chain. This will probably take long time like weeks to complete. If collateral is locked in the contract for more than a week, it can be considered as valid issue as per Sherlock ruling. + +Even if the collaterals are retrieved, there is still problem need to solve. The collateral tokens need to be sold in trading market to convert it to principal token. Remember the lenders as liquidity providers in LCG contract, the principal tokens are the tokens they provided, so in essence if they will convert their shares, they should receive principal tokens. So conversion of retrieve collaterals into principal tokens is necessary step in order to properly distribute the principal tokens to the lenders when they convert their shares. + +With all of these above processes involved, this is definitely will take a lengthy time and can take weeks or months while the lenders are waiting indefinitely. + +### Root Cause + +Liquidation process do not have capability to change the price of the loan (reduce the quantity of principal tokens) in order to encourage the liquidators to acquire the collateral assets in affordable way. To prove this, let's check all these liquidations functions. + 1. [liquidateDefaultedLoanWithIncentive](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L657-L747) - this is quite expensive , the loan to pay even starts with about 7x of amount due plus amount due initially if minimumamountdifference is more than zero. Aside from this, is the high usd price of principal token compare to collateral token. This will definitely turn away liquidators. + 2. [liquidateloanfull](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L787-L819) - this can be triggered after 24 hours since default and the total amount need to be paid is principal token plus interest but again it pertains to quantity of principal token. If the usd price of principal token is very high compare collateral, liquidators won't definitely pay for it. + + The protocol implement the pricing based on "quantity of the principal tokens" but did not consider the impact of significant USD pricing deviation between principal token and collateral token (high usd price of principal token against collateral token). Liquidators will never pay a very high loan for acquiring low valued collateral assets so liquidation may never happen. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +1. Significant increase in USD price of principal token compared to collateral token since the loan started. + +### Attack Path +This can be the scenario +image + + +1. Loan has been created and Lender Commitment group contract (LCG) serves as a lender. +2. Due to the sudden increase in usd price of principal token loaned, the borrower defaults on deadline repayment. +3. Liquidation is open for liquidators. +4. Liquidators can't do it due to imminent large loss. The loan payment is much higher than the collateral assets to be acquired in terms of USD. *Take note I did not include here the additional liquidation premium (about 7x at initial stage of liquidation) to simplify the explanation. If we add it, the principal token loan value is much higher*. +5. Liquidation is delayed and the collateral remain in the vault. +6. Weeks passed and still no liquidators are interested due to high loan price. +7. Lenders under LCG finally decided to close the loan and at least retrieve the collateral. However, the LCG contract as a lender, don't have access to close the loan and recover the collateral. +8. In one last desperate attempt, the lenders seek help from developers to upgrade the contract so the collateral can be retrieved. However, there are "other group of lenders" who are also affected, and the developers can't accomodate all at once. The lenders have no choice but forced to wait longer. +9. Even if the collateral is already retrieved, another step should be done and it should be converted into principal tokens and deposit to the LCG, so the lenders are finally able to exchange their shares and receive principal tokens. + +### Impact + +Significant delay in liquidation which basically "locked" the collateral until the liquidation process is completed. This could take weeks or months depending on market condition. No liquidator is interested to pay a very high loan compared to collateral assets. + +### PoC + +See attack path + +### Mitigation + +The liquidation process for LCG loan bids should have option during special situation wherein it can adjust the price of principal loan token (deduct some loan tokens to lower the price) which is comparable to the collateral assets to be acquired by liquidator in terms of USD.Make sure both parties (lenders and liquidators) has benefited from this liquidation process. \ No newline at end of file diff --git a/017.md b/017.md new file mode 100644 index 0000000..2b09e52 --- /dev/null +++ b/017.md @@ -0,0 +1,110 @@ +Bubbly Inky Sawfish + +High + +# Title: Incorrect implementation of `LenderCommitmentGroupShares._afterTokenTransfer()` may lead to DoS of `LenderCommitmentGroup_Smart` + +### Summary +Shareholders of `LenderCommitmentGroupShares` must call `prepareSharesForBurn` in advance to burn their shares. There is a cooldown mechanism in place for burning of their shares `LenderCommitmentGroupShares`. This mechanism utilizes the `LenderCommitmentGroupShares._afterTokenTransfer()` function to reset `poolSharesPreparedToWithdrawForLender` and `poolSharesPreparedTimestamp` each time a transfer or burn occurs. As a result, the holders of `LenderCommitmentGroupShares` cannot burn their shares for `withdrawDelayTimeSeconds` after any transfer or burn action. However, due to absence of a non-zero check in the `LenderCommitmentGroupShares._afterTokenTransfer()` function, anyone can reset `poolSharesPreparedToWithdrawForLender` and `poolSharesPreparedTimestamp` of other share holders by calling `transferFrom()` with a zero amount. This vulnerability allows attackers to capture all SharesPrepared events, effectively creating a DoS situation for burning shares. Consequently, while all shareholders can mint shares, they are unable to burn them, resulting in all funds staked in `LenderCommitmentGroup_Smart` being locked indefinitely. + +### Root Cause +Shareholders of `LenderCommitmentGroupShares` must call `prepareSharesForBurn` in advance to burn their shares. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L99-L118 +```solidity + function _prepareSharesForBurn( + address _recipient, + uint256 _amountPoolSharesTokens + ) internal returns (bool) { + + require( balanceOf(_recipient) >= _amountPoolSharesTokens ); + + poolSharesPreparedToWithdrawForLender[_recipient] = _amountPoolSharesTokens; + poolSharesPreparedTimestamp[_recipient] = block.timestamp; + + + emit SharesPrepared( + _recipient, + _amountPoolSharesTokens, + block.timestamp + + ); + + return true; + } +``` + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L37-L51 +```solidity + function burn(address _burner, uint256 _amount, uint256 withdrawDelayTimeSeconds) external onlyOwner { + + + //require prepared +@> require(poolSharesPreparedToWithdrawForLender[_burner] >= _amount,"Shares not prepared for withdraw"); +@> require(poolSharesPreparedTimestamp[_burner] <= block.timestamp - withdrawDelayTimeSeconds,"Shares not prepared for withdraw"); + + + //reset prepared + poolSharesPreparedToWithdrawForLender[_burner] = 0; + poolSharesPreparedTimestamp[_burner] = block.timestamp; + + + _burn(_burner, _amount); + } +``` + +There is a cooldown mechanism in place for burning of their shares `LenderCommitmentGroupShares`. This mechanism utilizes the `LenderCommitmentGroupShares._afterTokenTransfer()` function to reset `poolSharesPreparedToWithdrawForLender` and `poolSharesPreparedTimestamp` each time a transfer or burn occurs. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L60-L71 +```solidity + function _afterTokenTransfer( + address from, + address to, + uint256 amount + ) internal override { + + + //reset prepared +@> poolSharesPreparedToWithdrawForLender[from] = 0; +@> poolSharesPreparedTimestamp[from] = block.timestamp; + + } +``` +Shareholders of `LenderCommitmentGroupShares` are unable to burn their shares for `withdrawDelayTimeSeconds` after any transfer or burn action. However, the absence of a non-zero check for the transfer amount allows attackers to reset `poolSharesPreparedToWithdrawForLender` and `poolSharesPreparedTimestamp` for any account by calling `transferFrom()` with a zero amount. + +Attackers can create a DoS situation for burning shares by monitoring the SharesPrepared events emitted from `LenderCommitmentGroupShares` and repeatedly invoking `transferFrom()`. As a result, shareholders are unable to burn their shares forever. + +While the protocol owner could set `withdrawDelayTimeSeconds` to zero as a workaround, this would introduce another high-severity vulnerability, which is described [here](https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110). + +### Internal pre-conditions +none + +### External pre-conditions +none + +### Attack Path +1. Alice monitors all the `SharesPrepared` events emitted from `LenderCommitmentGroupShares`. +2. Each time she captures an event, Alice calls `transferFrom` with a zero amount. + +As a result, shareholders are unable to burn their shares indefinitely. + +### Impact +All funds deposited in `LenderCommitmentGroup_Smart` will be permanently locked. + +### PoC +none + +### Mitigation + +```diff + function _afterTokenTransfer( + address from, + address to, + uint256 amount + ) internal override { + ++ if (amount == 0) return; + //reset prepared + poolSharesPreparedToWithdrawForLender[from] = 0; + poolSharesPreparedTimestamp[from] = block.timestamp; + + } +``` \ No newline at end of file diff --git a/018.md b/018.md new file mode 100644 index 0000000..13cadf8 --- /dev/null +++ b/018.md @@ -0,0 +1,94 @@ +Sticky Bone Raven + +Medium + +# Off-by-One Error in _getStakeholdersForMarket Leads to Potential Memory Out-of-Bounds Access + +### Summary + +The function _getStakeholdersForMarket in the MarketRegistry.sol contains an off-by-one error when populating the stakeholders_ array. Specifically, the loop variable i is directly used to index into the array, which causes memory misalignment since the array's indices start from 0. This bug can result in out-of-bounds memory writes, corrupted data, or runtime exceptions. + +### Root Cause + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L1002 + +```Solidity +uint256 len = _set.length(); + +uint256 start = _page * _perPage; +if (start <= len) { + uint256 end = start + _perPage; + // Ensure we do not go out of bounds + if (end > len) { + end = len; + } + + stakeholders_ = new address[](end - start); + for (uint256 i = start; i < end; i++) { + stakeholders_[i] = _set.at(i); + } +} +``` + +The incorrect indexing logic in the loop: + +stakeholders_[i] = _set.at(i); +The loop iterates over the range [start, end) in _set, but directly uses i as the index for stakeholders_ without adjusting for the array's base index (0). + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Incorrect memory access may lead to: +- Reverts: If accessing an index out of bounds in stakeholders_. +- Corrupted Data: Assigning incorrect values or overwriting unintended memory locations. +- Gas Wastage: Due to failed transactions and repeated calls. + + +### PoC + +_No response_ + +### Mitigation + +Adjust the indexing logic in the loop to account for the zero-based indexing of stakeholders_. Replace: +stakeholders_[i] = _set.at(i); +With: +stakeholders_[i - start] = _set.at(i); + +Corrected Code: + +```Solidity +function _getStakeholdersForMarket( + EnumerableSet.AddressSet storage _set, + uint256 _page, + uint256 _perPage +) internal view returns (address[] memory stakeholders_) { + uint256 len = _set.length(); + + uint256 start = _page * _perPage; + if (start >= len) { + return new address } + + uint256 end = start + _perPage; + if (end > len) { + end = len; + } + + stakeholders_ = new address[](end - start); + for (uint256 i = start; i < end; i++) { + stakeholders_[i - start] = _set.at(i); // Correct index adjustment + } +} + +``` \ No newline at end of file diff --git a/019.md b/019.md new file mode 100644 index 0000000..a3e1409 --- /dev/null +++ b/019.md @@ -0,0 +1,52 @@ +Strong Butter Crow + +Medium + +# uses `ERC20.approve` instead of safe approvals, causing it to always revert on some ERC20s + +### Summary + +the "default" ERC20 behavior expects the `approve` function to return a boolean, however, some ERC20s on some chains don't return a value. +The most popular example is USDT on the main net, and as the docs mention it should be compatible on any EVM chain and will support USDT: + +> On what chains are the smart contracts going to be deployed? + +Ethereum Mainnet, Polygon PoS, Arbitrum One, Base + +> If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of [weird tokens](https://github.com/d-xo/weird-erc20) you want to integrate? + +For LenderGroups (our primary concern for this audit - LenderCommitmentGroup_Smart), we are allowing any token complying with the ERC20 standard except for fee-on-transfer tokens. Pools will be deployed with the principal token and collateral token types ( both must be ERC20) already defined and visible on the frontend so borrowers/lenders will be able to vet tokens for weirdness that way and avoid them if needed. Therefore we do not especially care if a 'weird' token breaks the contract since pools with tokens that are obviously weird will be naturally avoided by users but we would like to know if weird tokens affects do break our pools . We absolutely do want to fully support all mainstream tokens like USDC, USDT, WETH, MOG, PEPE, etc. and we already took special consideration to make sure USDT works with our contracts. + + +### Root Cause + +Some known tokens don't return a value on approvals, more info [[here](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#missing-return-values)](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#missing-return-values), an example of this is USDT, which is mentioned that the protocol will use it. + +```solidity + principalToken.approve(address(TELLER_V2), _principalAmount); +``` +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L556 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +_No response_ + +### PoC + +_No response_ + +### Mitigation + +Use `safeApprove` instead of `approve`. \ No newline at end of file diff --git a/020.md b/020.md new file mode 100644 index 0000000..dd08c62 --- /dev/null +++ b/020.md @@ -0,0 +1,105 @@ +Harsh Yellow Poodle + +Medium + +# Incorrect Price Calculation for Negative Ticks Due to Missing Rounding Down + +### Summary + +The function `getSqrtTwapX96` is used to calculate the Time-Weighted Average Price (TWAP) for a given Uniswap V3 pool. It fetches the cumulative tick values over a specified interval and calculates the square root price in X96 format. However, the function does not handle negative tick differences correctly, leading to potential inaccuracies in the TWAP calculation. + +In cases where `(tickCumulatives[1] - tickCumulatives[0])` is negative and `(tickCumulatives[1] - tickCumulatives[0]) % twapInterval != 0`, the tick should be rounded down to account for fractional remainders. The current implementation directly performs integer division, which implicitly rounds towards zero, leading to an incorrectly higher tick value and therefore inaccurate price calculations. + +### Root Cause + +The implementation of [getSqrtTwapX96](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/UniswapPricingLibrary.sol#L107-L120) does not apply rounding towards negative infinity for the tick calculation when `(tickCumulatives[1] - tickCumulatives[0])` is negative. +```solidity + uint32[] memory secondsAgos = new uint32[](2); + secondsAgos[0] = twapInterval + 1; // from (before) + secondsAgos[1] = 1; // one block prior + + (int56[] memory tickCumulatives, ) = IUniswapV3Pool(uniswapV3Pool) + .observe(secondsAgos); + + // tick(imprecise as it's an integer) to price + sqrtPriceX96 = TickMath.getSqrtRatioAtTick( + int24( + (tickCumulatives[1] - tickCumulatives[0]) / + int32(twapInterval) + ) + ); +``` +As result, in case if `(tickCumulatives[1] - tickCumulatives[0])` is negative and `(tickCumulatives[1] - tickCumulatives[0]) % twapInterval != 0`, then returned tick will be bigger then it should be, hence incorrect prices would be used. + +In contrast, the [Uniswap OracleLibrary](https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/libraries/OracleLibrary.sol#L16-L41) includes a safeguard for such cases by explicitly decrementing the tick when necessary. +```solidity + int56 tickCumulativesDelta = tickCumulatives[1] - tickCumulatives[0]; + uint160 secondsPerLiquidityCumulativesDelta = + secondsPerLiquidityCumulativeX128s[1] - secondsPerLiquidityCumulativeX128s[0]; + + arithmeticMeanTick = int24(tickCumulativesDelta / secondsAgo); + // Always round to negative infinity +@> if (tickCumulativesDelta < 0 && (tickCumulativesDelta % secondsAgo != 0)) arithmeticMeanTick--; +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +The TWAP derived from the function will be inaccurate when the cumulative tick difference is negative and not evenly divisible by the interval. In such cases, the tick is effectively rounded up instead of down, causing the returned price to be higher than the actual value. + +### PoC + +_No response_ + +### Mitigation + +Similar to Uniswap, add the following check before using tick to calculate sqrtPriceX96. +```diff + function getSqrtTwapX96(address uniswapV3Pool, uint32 twapInterval) + internal + view + returns (uint160 sqrtPriceX96) + { + if (twapInterval == 0) { + // return the current price if twapInterval == 0 + (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(uniswapV3Pool).slot0(); + } else { + uint32[] memory secondsAgo = new uint32[](2); + secondsAgo[0] = _interval; // from (before) + secondsAgo[1] = 0; // to (now) + + (int56[] memory tickCumulatives, ) = IUniswapV3Pool(_uniswapV3Pool) + .observe(secondsAgo); + + ++ int56 tickCumulativeDelta = tickCumulatives[1] - tickCumulatives[0]; + ++ int24 tick = int24(tickCumulativeDelta / int56(int32(twapInterval))); + + // Apply rounding towards negative infinity when necessary ++ if (tickCumulativeDelta < 0 && (tickCumulativeDelta % int32(twapInterval) != 0)) tick--; + ++ sqrtPriceX96 = TickMath.getSqrtRatioAtTick(tick); + +- sqrtPriceX96 = TickMath.getSqrtRatioAtTick( +- int24( +- (tickCumulatives[1] - tickCumulatives[0]) / +- int32(twapInterval) +- ) +- ); + + } + + } +``` \ No newline at end of file diff --git a/021.md b/021.md new file mode 100644 index 0000000..986c673 --- /dev/null +++ b/021.md @@ -0,0 +1,79 @@ +Real Honey Poodle + +Medium + +# wrong implement of 'revalidateCollateral' + +### Summary + +Here in the revalidateCollateral we are not checking whether borrower is zero address or not,then we are calling the _checkBalance and there we are verifying with the IERC20(_collateralInfo._collateralAddress).balanceOf( + _borrowerAddress).so we are checking the balance of zero address. + +### Root Cause + + https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L185 + +function revalidateCollateral(uint256 _bidId) + external view + returns (bool validation_) + { + Collateral[] memory collateralInfos = getCollateralInfo(_bidId); + address borrower = tellerV2.getLoanBorrower(_bidId); + (validation_, ) = _checkBalances(borrower, collateralInfos, true); + } + + + + function _checkBalance( + address _borrowerAddress, + Collateral memory _collateralInfo + ) internal virtual view returns (bool) { + CollateralType collateralType = _collateralInfo._collateralType; + + if (collateralType == CollateralType.ERC20) { + return + _collateralInfo._amount <= + IERC20(_collateralInfo._collateralAddress).balanceOf( + _borrowerAddress + ); + } else if (collateralType == CollateralType.ERC721) { + return + _borrowerAddress == + IERC721Upgradeable(_collateralInfo._collateralAddress).ownerOf( + _collateralInfo._tokenId + ); + } else if (collateralType == CollateralType.ERC1155) { + return + _collateralInfo._amount <= + IERC1155Upgradeable(_collateralInfo._collateralAddress) + .balanceOf(_borrowerAddress, _collateralInfo._tokenId); + } else { + return false; + } + } + + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +_No response_ + +### PoC + +_No response_ + +### Mitigation + + require(borrower != address(0), "Loan has no borrower"); \ No newline at end of file diff --git a/022.md b/022.md new file mode 100644 index 0000000..642665e --- /dev/null +++ b/022.md @@ -0,0 +1,78 @@ +Salty Amethyst Caribou + +High + +# Bids canceled or accepted are re-acceptable + +### Summary + +Once Bids are accepted, the state is changed from PENDING to others. Moreover if the borrower canceled the bid, the state is changed to CANCELED. But despite of this, these types of bids are re-acceptable. + +### Root Cause + +In [`TellerV2.sol::lenderAcceptBid#L500`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L500), it checks to see if a pending loan has expired. +```solidity +function lenderAcceptBid(uint256 _bidId) + external + override + pendingBid(_bidId, "lab") + whenProtocolNotPaused + returns ( + uint256 amountToProtocol, + uint256 amountToMarketplace, + uint256 amountToBorrower + ) +{ + ... +@> require(!isLoanExpired(_bidId), "BE"); + ... +} +``` +However, in `isLoanExpired()` function, it returns `false` if the `bid.state` is not equal to `BidState.PENDING` and if `bidExpirationTime[_bidId]` is equal to 0.[`TellerV2.sol::isLoanExpired#L1176-L1184`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L1176C5-L1184C6) +```solidity + function isLoanExpired(uint256 _bidId) public view returns (bool) { + Bid storage bid = bids[_bidId]; + +@> if (bid.state != BidState.PENDING) return false; +@> if (bidExpirationTime[_bidId] == 0) return false; + + return (uint32(block.timestamp) > + bid.loanDetails.timestamp + bidExpirationTime[_bidId]); + } +``` +Following this function, if the state of Bids aren't `PENDING`,for example Bids were already canceled by borrower or already accepted once, it returns `false` and the require in `lenderAcceptBid()#L500` is passed. The case of `bidExpirationTime[_bidId] == 0` is also passed. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +If the accepted bids are accepted again, previous lenders lost their assets and borrowers also lost their totalRepaid assets. + +### PoC + +_No response_ + +### Mitigation + +Fix `isLoanExpired()` function by following: +```solidity + function isLoanExpired(uint256 _bidId) public view returns (bool) { + Bid storage bid = bids[_bidId]; + +@> if (bid.state != BidState.PENDING) return true; +@> if (bidExpirationTime[_bidId] == 0) return true; + + return (uint32(block.timestamp) > + bid.loanDetails.timestamp + bidExpirationTime[_bidId]); + } +``` \ No newline at end of file diff --git a/023.md b/023.md new file mode 100644 index 0000000..c0be8bc --- /dev/null +++ b/023.md @@ -0,0 +1,144 @@ +Custom Pineapple Newt + +High + +# Lenders can force borrowers into liquidation + +### Summary + +## Summary +Adversaries can still forcefully revert loan repayment attempts by reverting the try-catch in `_sendOrEscrowFunds` +## Description +Last audit had a [similar finding](https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/178) where loan lenders could set the `repayLoanCallback` address to an unimplemented contract which would cause the try-catch to revert in the `try`, not getting to the `catch`. The implemented fix was to modify `setRepaymentListenerForBid` to check whether the input `_listener` address has any code behind it. +```solidity + function setRepaymentListenerForBid(uint256 _bidId, address _listener) external { + uint256 codeSize; + assembly { + codeSize := extcodesize(_listener) + } + require(codeSize > 0, "Not a contract"); + address sender = _msgSenderForMarket(bids[_bidId].marketplaceId); + + + require( + sender == getLoanLender(_bidId), + "Not lender" + ); + + + repaymentListenerForBid[_bidId] = _listener; + } +``` +This fix seems reasonable at first glance and more can be [read about it here](https://ethereum.stackexchange.com/questions/129150/solidity-try-catch-call-to-external-non-existent-address-method). Reason why no code causes `try` to revert: +>What's really happening is that you are expecting a return value, which is never returned because nothing ever gets executed since there is no code to execute. + +However code size is checked only in the setter and not during `_sendOrEscrowFunds` execution. It is possible the `_listener` address to have code during the setter execution and no code during loan repayment via a `selfdestruct` inbetween. After [EIP-6780](https://eips.ethereum.org/EIPS/eip-6780), `selfdestruct` was designed to delete the code only if invoked within the same transaction as its' creation. The attack works the following way in 1 transaction: +1. Deploy decoy `listener` contract with `selfdestruct` in it +2. Invoke `setRepaymentListenerForBid` with deployed `listener` address +3. Invoke `selfdestruct` in the decoy + +Adversary will be able to set a `_listener` address which has code during setter execution and have no code afterwards. Borrowers will be unable to pay their loans and be forced to default since `try-catch` will try to access unimplemented contract, causing the `try` to revert. + +This attack's PoC can only be written in Remix due to a foundry limitation of performing all steps within 1 transaction, so if we check `extcodesize(listener)` right after the `selfdestruct` it will still report a non-0 value due to its' [geth implementation](https://github.com/ethereum/go-ethereum/blob/c52def7f114aa48f50ed9956bc9661550300addb/core/state/statedb.go#L433). More on this topic can be found [here](https://github.com/foundry-rs/foundry/issues/5781). +Once the state finalizes, `extcodesize(listener)` will return a null value. + +### Root Cause + +In `TellerV2._sendOrEscrowFunds`, the `loanRepaymentListener` is not checked for having a non-0 code size before being called. + +### Internal pre-conditions + +None + +### External pre-conditions + +None + +### Attack Path + +1. Malicious lender deploys decoy `listener` contract with `selfdestruct` in it +2. Malicious lender invokes `setRepaymentListenerForBid` with deployed `listener` address +3. Malicious lender invokes `selfdestruct` in the decoy +4. Later in time, victim borrower attempts to repay their loan - revert + +### Impact + +- Borrowers are unable to repay their loans +- Borrowers are forced to get liquidated and have their collateral seized +- Lenders force defaults and seize collateral + +### PoC + +**Note that this PoC is written to test in Remix due to Foundry limitations described in the main body** + +The PoC can be ran 2 different ways: +1. Deploy `MyTest` -> invoke `testAttack` (pass) -> invoke `attack` (fail, attempt to access a method to unimplemented contract) +2. Deploy `Attack` -> invoke `deployBomb` (will return random address and size) -> can be double checked by calling the 2 public storage variables -> invoke `checkSizeAfterExec` -> will always return 0 (proof no code exists anymore) +```solidity +// SPDX-License-Identifier: GPL-3.0 + +pragma solidity 0.8.26; + +contract Bomb { + function destroy() public { + address payable addr = payable(0); + selfdestruct(addr); + } +} + +contract Attack { +uint public storageSize; +Bomb public storageNb; + function deployBomb() public returns (address, uint) { // deploys and destroys a bomb + Bomb nb = new Bomb(); // deploys a Bomb + uint size; + assembly { + size := extcodesize(nb) // check nb (newbomb) size + } + // console.log(size); + nb.destroy(); // invoke selfdestruct + storageSize = size; + storageNb = nb; + return (address(nb), size); // return addr and size (will be positive) + } + function checkSizeAfterExec() public view returns (uint) { + uint newSize; + Bomb memoryNb = storageNb; + assembly { + newSize := extcodesize(memoryNb) // will always return 0 as it checks the code behind the destroyed bomb + } + return newSize; + } +} + +contract MyTest { + address public nb; + function testAttack() public { + Attack na = new Attack(); // deploy new attack contract + (nb,) = na.deployBomb(); // deploy and destroy a bomb + address nb1 = nb; + uint size; + assembly { + size := extcodesize(nb1) // check the size of the just destroyed contract + } + assert(size > 0); // assert its non-0 +// console.log(size); + + Bomb n1 = Bomb(nb); // wrap the destroyed bomb + try n1.destroy() {} // should always pass as there is still code behind it + catch {} + + } + + function attack() public { // always reverts here + Bomb n1 = Bomb(nb); + try n1.destroy(){} // attempts a try-catch on the contract deployed and destroyed above + catch {} // however it has no code anymore and reverts every time + } +} +``` + + +### Mitigation + +Check the code size of the `listener` address just before the callback in `sendOrEscrowFunds` and skip calling it if it's 0. \ No newline at end of file diff --git a/024.md b/024.md new file mode 100644 index 0000000..8b46191 --- /dev/null +++ b/024.md @@ -0,0 +1,80 @@ +Custom Pineapple Newt + +High + +# Lender group members can be prevented from burning their shares forever + +### Summary + +## Summary +Adversaries can constantly reset the withdrawal delay of lender group members by performing 0-value `transferFrom` transactions to invoke the `afterTokenTransfer` hook. +## Description +Currently there is a delay on withdrawals to prevent sandwich attacks in lender group contracts. Members must invoke `prepareSharesForBurn` by stating how many shares they want to burn and start an internal countdown. Afterwards, members invoke `burnSharesToWithdrawEarnings` which checks whether the delay passed in `burn` +```solidity + function burn(address _burner, uint256 _amount, uint256 withdrawDelayTimeSeconds) external onlyOwner { + + //require prepared + require(poolSharesPreparedToWithdrawForLender[_burner] >= _amount,"Shares not prepared for withdraw"); + @> require(poolSharesPreparedTimestamp[_burner] <= block.timestamp - withdrawDelayTimeSeconds,"Shares not prepared for withdraw"); + + + //reset prepared + poolSharesPreparedToWithdrawForLender[_burner] = 0; + poolSharesPreparedTimestamp[_burner] = block.timestamp; + + + + _burn(_burner, _amount); + } +``` + + +This countdown is reset every time a member invokes a share transfer through the `_afterTokenTransfer` hook presumably to prevent users preparing shares in advance by transferring it between one another. +```solidity + function _afterTokenTransfer( + address from, + address to, + uint256 amount + ) internal override { + + //reset prepared + poolSharesPreparedToWithdrawForLender[from] = 0; + poolSharesPreparedTimestamp[from] = block.timestamp; + + + } +``` +Adversaries can perform 0-value `transferFrom` transactions which will always pass as there are no 0-value checks in OZ's version 4.8 `ERC20.sol` used by the protocol. Users will have their countdown constantly reset thus being prevented from withdrawing forever or until a bribe is paid. + +### Root Cause + +- OpenZeppelin's `ERC20.transferFrom` has no 0-value input validation +- `LenderCommitmentGroupShares._afterTokenTransfer` does not perform 0-value input either. + +### Internal pre-conditions + +Group members must have invoked `prepareSharesForBurn` + +### External pre-conditions + +None + +### Attack Path + +1. Group member invokes `prepareSharesForBurn` starting the countdown +2. Adversary invokes `transferFrom(victim, to, 0)` minutes before the cooldown expires +3. Cooldown and shares are reset because `_afterTokenTransfer` was triggered +4. Group member is forced to re-prepare their shares +5. Attacker repeats this continuously or until a bribe is paid + +### Impact + +Lender commit group members will have their funds permanently locked in the contract + +### PoC + +_No response_ + +### Mitigation + +Rewrite the [`_afterTokenTransfer`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L60) hook to be skipped in case of `amount = 0` \ No newline at end of file diff --git a/025.md b/025.md new file mode 100644 index 0000000..264bac3 --- /dev/null +++ b/025.md @@ -0,0 +1,41 @@ +Custom Pineapple Newt + +Medium + +# Market owners can still front-run users to increase fees + +### Summary + +Issue [#125](https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/125)'s fix from the previous Sherlock audit still allows market owners to front-run users and increase fees. + +### Root Cause + +In `TellerV2.sol`, users [submitting a bid can still be front-ran](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L517-L519) by the market owner, increasing the fee and forcing them to pay a higher fee amount. + +### Internal pre-conditions + +None + +### External pre-conditions + +None + +### Attack Path + +1. User submits bid +2. Market owner front-runs the user's tx with call to [`setMarketFeePercent`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L637-L653) +3. User gets charged higher fee + +### Impact + +The issue is that the fix from the previous audit simply limited the market owners to a max fee percentage of 10%. This still allows the front-running issue to happen. For example the market fee is 1%, but the owner front runs the user's call to submit bid and increases it to 10%, the user now pays 10x more in fees. + +Thus, the "fix" of the previous issue doesn't remedy users from still experiencing the loss due to a fee increase, it's just now limited to 10x instead of 100x. + +### PoC + +_No response_ + +### Mitigation + +An option would be a timelock. When a market owner increases/decreases the fee, don't apply the changes until 24h later or something similar. \ No newline at end of file diff --git a/026.md b/026.md new file mode 100644 index 0000000..11425b5 --- /dev/null +++ b/026.md @@ -0,0 +1,42 @@ +Gentle Turquoise Shetland + +High + +# Marketplace fees are not bound to bid at submission time, which might cause unexpected fees + +### Summary + +Marketplace fees are only taken into account for a bid when a lender calls [lenderAcceptBid](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L473), which allows the marketplace owner to freely modify the marketplace fee before the bid is actually accepted by the lender. + +### Root Cause + +The fee for the bid is fetched from the marketplace’s details in lenderAcceptBid. In the time interval between a user submits the bid and a lender accepts the bid, the marketplace owner can change the fee without notifying the user. This can be used as a honeypot where one creates a marketplace with very low fees, waits for users to submit bids, changes the fee to a very high percent and accepts the bids, causing users to pay more than they think. + +### Internal pre-conditions + +1. Attacker needs to create a marketplace +2. Victim needs to create a bid + +### External pre-conditions + +_No response_ + +### Attack Path + +1. [Attacker] Creates a marketplace with a low fee +2. [Victim] Submits a bid +3. [Attacker] Changes the marketplace fee to a higher amount +4. [Attacker] Accepts the bid (either from their own contract or a new one added to the list of verified lenders) +5. [Victim] Has a bid with a higher fee than expected + +### Impact + +This issue allows attackers to gain additional tokens by tricking users into submitting a bid to a rogue marketplace with apparent low fees, which will have its fee changed further on without notifying the user. + +### PoC + +_No response_ + +### Mitigation + +Introduce a new field in the Bid structure that stores the marketplace fee at the time of bid submission. Then, lenderAcceptBid reads the value directly from the structure. \ No newline at end of file diff --git a/027.md b/027.md new file mode 100644 index 0000000..9edd083 --- /dev/null +++ b/027.md @@ -0,0 +1,65 @@ +Custom Pineapple Newt + +High + +# Early repayers can be unexpectedly liquidated + +### Summary + +## Summary +`dueDate` calculations currently punish users who pay a cycle early as it does not extend their next due date +## Description +Let's observe how due dates are calculated in [`V2Calculations.sol`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/V2Calculations.sol#L179) +```solidity + else if (_bidPaymentCycleType == PaymentCycleType.Seconds) { + + dueDate_ = _acceptedTimestamp + _paymentCycle; + + uint32 delta = _lastRepaidTimestamp - _acceptedTimestamp; + if (delta > 0) { + uint32 repaymentCycle = uint32( + Math.ceilDiv(delta, _paymentCycle) + ); + dueDate_ += (repaymentCycle * _paymentCycle); + } + } +``` +It starts by assigning `dueDate_` the value of the very first expected repayment and adds an extra `repaymentCycle` every time 1 `_paymentCycle` passes. + +Issue is that `repaymentCycle` will return the same value for 2 cycle payments that took place within the same period e.g: +Assume 10 payment periods, each period is 10 seconds, entire loan duration is 100 seconds, accepted timestamp is 0 +1. Payment with `_lastRepaidTimestamp = 6` will return `delta = 6, repaymentCycle = ceilDiv(6,10) = 1, dueDate_ = 20` +2. Payment with `_lastRepaidTimestamp = 7` will return `delta = 7, repaymentCycle = ceilDiv(7,10) = 1, dueDate_ = 20` + +Users who wish to repay earlier either for good reputation or to pay less interest in the future will be punished for doing so and could be unexpectedly liquidated since they know they have pre-paid their obligation for the subsequent cycle and still be marked as defaulted. A more illustrative example would be to pre-pay your April rent during February and still get evicted couple days into March for missing a payment. + +### Root Cause + +- In `V2Calculations.calculateNextDueDate` does not extend the next due date based on the number of already paid cycles + +### Internal pre-conditions + +Borrower pre-pays their dues for a period further in the future (e.g paying April rent during February) + +### External pre-conditions + +None + +### Attack Path + +1. Borrower pays 2 periodic payments earlier in order to retain good reputation and pay interest on smaller principal in the future +2. Borrower does not initiate any payments for 1 full cycle +3. Arbitrary party liquidates the borrower for missing a payment and seizes their collateral +4. Borrower is liquidated despite paying early + +### Impact + +Unfair liquidations and collateral seizure + +### PoC + +_No response_ + +### Mitigation + +Track the total number of cycles paid and use it to calculate future due dates \ No newline at end of file diff --git a/028.md b/028.md new file mode 100644 index 0000000..37ba677 --- /dev/null +++ b/028.md @@ -0,0 +1,48 @@ +Magic Rainbow Locust + +High + +# Storage collision in contract being upgraded due to unexpected storage usage + +### Summary + +The introduction of unexpected storage variables in `TellerV2Context` will cause a storage slot collision leading to unauthorized modification of the protocol’s critical data as an attacker who controls the upgrade path can deploy a malicious upgrade to overwrite existing variables. + + + +### Root Cause + +In [TellerV2Storage.sol:133-139](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2Storage.sol#L133-L139), the TellerV2Context contract, which was not intended to introduce storage variables, declares mappings such as _trustedMarketForwarders and _approvedForwarderSenders. These additions are not accounted for in the inherited storage layout, causing storage slot collisions and allowing malicious upgrades to overwrite critical existing state. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. The attacker (who controls or compromises the entity responsible for upgrades) proposes a new implementation contract of `TellerV2Context` that includes malicious storage variables. +2. Upon upgrade, the introduced storage variables occupy the same slots as previously declared variables in `TellerV2Storage`. +3. The original critical data (addresses, configuration parameters, etc.) is overwritten by the attacker’s chosen values. +4. With the data overwritten, the attacker gains unauthorized permissions or access to privileged functions, allowing them to extract funds or alter system behavior at will. + +### Impact + +The protocol and its users suffer complete compromise of their expected state integrity. Critical parameters, roles, and balances can be overwritten, potentially enabling the attacker to: + +- Grant themselves administrative privileges +- Redirect fee payments or loan proceeds +- Misrepresent repayment or collateral requirements + +This can lead to severe financial loss for lenders, borrowers, and the protocol’s treasury. While the attacker gains privileged access and can siphon funds, the protocol’s stakeholders lose trust and value. + +### PoC + +_No response_ + +### Mitigation + +_No response_ \ No newline at end of file diff --git a/029.md b/029.md new file mode 100644 index 0000000..8a4c44b --- /dev/null +++ b/029.md @@ -0,0 +1,133 @@ +Brisk Sapphire Chipmunk + +Medium + +# ERC20.approve Used Instead of Safe Approvals, Causing Pool Failures with Some ERC20s + +### Summary + +The function [acceptFundsForAcceptBid](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L518) handles accepting a loan bid, transferring funds to the borrower, and securing collateral. It approves the TELLER_V2 contract to transfer the loan's principal amount using "ERC20.approve". + +However, some ERC20s on some chains don't return a value. +The most popular example is USDT and USDC on the main net, and as the docs mention it should be compatible on any EVM chain and will support USDT: + +> Q: On what chains are the smart contracts going to be deployed? +> Ethereum Mainnet, Polygon PoS, Arbitrum One, Base + +> Q: If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of [[weird tokens](https://github.com/d-xo/weird-erc20)](https://github.com/d-xo/weird-erc20) you want to integrate? +> We absolutely do want to fully support all mainstream tokens like USDC, USDT, WETH, MOG, PEPE, etc. and we already took special consideration to make sure USDT works with our contracts. + +Despite this claim, the current implementation of the approve function does not account for tokens like USDT, contradicting the protocol's intentions. +Therefore [acceptFundsForAcceptBid](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L518) will never work on the EVM Chain or other related chain and tokens. + +### Root Cause + +In LenderCommitmentGroup_Smart.soL: 556, approve function is used instead of safeApprove. USDT on the main net doesn't return a value, . This includes USDC which should work in the protocol. +This behavior causes the approve function to revert when interacting with these tokens. + +```solidity + function acceptFundsForAcceptBid( + address _borrower, + uint256 _bidId, + uint256 _principalAmount, + uint256 _collateralAmount, + address _collateralTokenAddress, + uint256 _collateralTokenId, + uint32 _loanDuration, + uint16 _interestRate + ) external onlySmartCommitmentForwarder whenForwarderNotPaused whenNotPaused { + + require( + _collateralTokenAddress == address(collateralToken), + "Mismatching collateral token" + ); + //the interest rate must be at least as high has the commitment demands. The borrower can use a higher interest rate although that would not be beneficial to the borrower. + require(_interestRate >= getMinInterestRate(_principalAmount), "Invalid interest rate"); + //the loan duration must be less than the commitment max loan duration. The lender who made the commitment expects the money to be returned before this window. + require(_loanDuration <= maxLoanDuration, "Invalid loan max duration"); + + require( + getPrincipalAmountAvailableToBorrow() >= _principalAmount, + "Invalid loan max principal" + ); + + + uint256 requiredCollateral = calculateCollateralRequiredToBorrowPrincipal( + _principalAmount + ); + + + + require( + _collateralAmount >= + requiredCollateral, + "Insufficient Borrower Collateral" + ); + + principalToken.approve(address(TELLER_V2), _principalAmount); +//do not have to override msg.sender as this contract is the lender ! + _acceptBidWithRepaymentListener(_bidId); + + totalPrincipalTokensLended += _principalAmount; + + activeBids[_bidId] = true; //bool for now + + + emit BorrowerAcceptedFunds( + _borrower, + _bidId, + _principalAmount, + _collateralAmount, + _loanDuration, + _interestRate + ); + } +``` +The specific part of the function is highlighted her; +```solidity + + principalToken.approve(address(TELLER_V2), _principalAmount); + + //do not have to override msg.sender as this contract is the lender ! +``` + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L556 +### + + +### Internal pre-conditions + +1. The protocol uses approve calls in functions like acceptFundsForAcceptBid. +2. There is no consideration or error handling for approve calls when interacting with USDT, USDC. + +### External pre-conditions + +1. The protocol operates on EVM-compatible chains where these tokens are prevalent. + + +### Attack Path + +1. Users or lenders supply USDT or USDC as the principalToken. +2. Transactions that include approve calls revert, causing the protocol to fail in providing the intended functionality. + +### Impact + +1. The protocol becomes unusable with ERC20 tokens like USDT AND USDC, a widely used stablecoin. +2. Breaks the intended functionality of the protocol. +3. This breaks a criticial function in the protocol and causes the pool to fail. + +### PoC + +_No response_ + +### Mitigation + +Use `safeApprove` instead of `approve` +```solidity +require( + _collateralAmount >= + requiredCollateral, + "Insufficient Borrower Collateral" + ); +principalToken.safeApprove(address(TELLER_V2), _principalAmount); +``` \ No newline at end of file diff --git a/030.md b/030.md new file mode 100644 index 0000000..c6ecb44 --- /dev/null +++ b/030.md @@ -0,0 +1,63 @@ +Magic Rainbow Locust + +High + +# Uniswap Price Oracle Manipulation Leading to Under-Collateralized Loans + +### Summary + +The reliance on an unprotected Uniswap TWAP price oracle in `LenderCommitmentGroup_Smart.sol` will cause under-collateralized loans for lenders, as an attacker can manipulate the Uniswap pool price to artificially reduce the required collateral amount and borrow more principal tokens than they should, potentially leading to loss of funds for the lending pool. + + +### Root Cause + +In `LenderCommitmentGroup_Smart.sol`, specifically in the function `calculateCollateralTokensAmountEquivalentToPrincipalTokens, the contract uses the Uniswap V3 TWAP price oracle obtained from `UniswapPricingLibrary.getUniswapPriceRatioForPoolRoutes(poolOracleRoutes)` without sufficient safeguards against price manipulation. The lack of a minimum enforced TWAP interval and absence of limits on the exchange rate `(maxPrincipalPerCollateralAmount defaulting to zero)` allow an attacker to manipulate the oracle price, reducing the required collateral for loans. + + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L850 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + + +The attacker examines the `poolOracleRoutes` configuration in the `LenderCommitmentGroup_Smart` contract to identify the Uniswap V3 pool(s) used as the price oracle between the principal and collateral tokens. By analyzing these routes, the attacker locates specific pools with low liquidity, making them susceptible to price manipulation. + +The attacker manipulates the Uniswap pool's price by executing large trades that decrease the price of the collateral token relative to the principal token. Specifically, the attacker swaps a significant amount of principal tokens for collateral tokens in the Uniswap pool, causing the collateral token's price to drop sharply due to the low liquidity. + +Due to the short Time-Weighted Average Price (TWAP) interval configured in the `poolOracleRoutes` (e.g., 3 seconds), the manipulated price quickly affects the oracle price returned by `UniswapPricingLibrary.getUniswapPriceRatioForPoolRoutes(poolOracleRoutes)`. This manipulated oracle price leads to an inflated `principalPerCollateralAmount`, which is calculated using the `calculateCollateralTokensAmountEquivalentToPrincipalTokens` function in the `LenderCommitmentGroup_Smart` contract. + +As a result, the required collateral amount for borrowing is significantly reduced. The attacker calls `acceptSmartCommitmentWithRecipient()` through the `SmartCommitmentForwarder` contract to request a large loan, providing only the reduced amount of collateral tokens, as calculated using the manipulated price. + +The `LenderCommitmentGroup_Smart` contract approves the loan based on the reduced collateral requirement, and the attacker receives the principal amount while having provided insufficient collateral. The attacker then defaults on the loan, keeping the excess principal tokens. When the loan defaults, the collateral seized by the lending pool is insufficient to cover the outstanding principal and interest, leading to financial losses for the lenders. + +**Proof of Concept (PoC)** + + +### Impact + +The lending pool suffers a financial loss as loans are under-collateralized due to manipulated oracle prices. The attacker gains excess principal tokens without providing adequate collateral. If the attacker defaults on the loan, the collateral seized will not cover the outstanding principal and interest, leading to potential significant losses for lenders. + + +### PoC + + +Suppose the lending pool relies on a Uniswap V3 pool between the principal token (e.g., USDC) and the collateral token (e.g., DAI), using a very short TWAP interval of 3 seconds. + +The attacker swaps a large amount of USDC for DAI in the Uniswap pool, significantly decreasing the price of DAI relative to USDC. Due to the low liquidity and large trade size, this causes a substantial price impact. + +Given the short TWAP interval, the manipulated price quickly reflects in the oracle price obtained by calling `UniswapPricingLibrary.getUniswapPriceRatioForPoolRoutes(poolOracleRoutes)`. The attacker then calculates the reduced collateral requirement by invoking `calculateCollateralTokensAmountEquivalentToPrincipalTokens(principalAmount)` in the `LenderCommitmentGroup_Smart` contract. This function uses the inflated `principalPerCollateralAmount`, resulting in a much lower required collateral amount for the desired principal loan. + +The attacker proceeds to request a loan by calling `acceptSmartCommitmentWithRecipient()` via the `SmartCommitmentForwarder`, supplying only the reduced amount of DAI as collateral. The lending pool, relying on the manipulated oracle price, approves the loan based on the calculated collateral requirement. + +Finally, the attacker defaults on the loan, retaining the borrowed USDC. The collateral seized by the lending pool is insufficient to cover the outstanding loan amount due to the artificially lowered collateral requirement, leading to a financial loss for the lenders. + +### Mitigation + +_No response_ \ No newline at end of file diff --git a/031.md b/031.md new file mode 100644 index 0000000..319b13a --- /dev/null +++ b/031.md @@ -0,0 +1,103 @@ +Custom Pineapple Newt + +Medium + +# Overpaying a loan can DoS an entire lender commitment group + +### Summary + +## Summary +Inserting larger amount in `repayLoan` allows to repay more than what was lent to user, disturbing the internal accounting of a lender group +## Description +Lender commitment groups use 2 variables to track how many tokens are currently being lent out - `totalPrincipalTokensLended` and `totalPrincipalTokensRepaid`. +Effectively, the latter should always be smaller or equal than the former. Whenever the 2 variables are equal it means that there are currently no active loans which is used in `getTotalPrincipalTokensOutstandingInActiveLoans` +```solidity + function getTotalPrincipalTokensOutstandingInActiveLoans() + public + view + returns (uint256) + { + return totalPrincipalTokensLended - totalPrincipalTokensRepaid; + } +``` + +This method can be forced to always revert if user repays at least 1 wei more than they were originally lent, causing an underflow during substraction. +The code currently allows for repaying more due to a logic error within `_repayLoan` +The method is passed a `Payment` struct which consists of 2 variables whose sum totals the `amount` borrower is willing to overpay (principal + interest) +```solidity + function _repayLoan( + uint256 _bidId, + @> Payment memory _payment, + uint256 _owedAmount, + bool _shouldWithdrawCollateral + ) internal virtual { + Bid storage bid = bids[_bidId]; +@> uint256 paymentAmount = _payment.principal + _payment.interest; // memory variable summing the 2 values + + RepMark mark = reputationManager.updateAccountReputation( + bid.borrower, + _bidId + ); + // Check if we are sending a payment or amount remaining + if (paymentAmount >= _owedAmount) { +@> paymentAmount = _owedAmount; // memory variable is overwritten to match the owed amount + + if (bid.state != BidState.LIQUIDATED) { + bid.state = BidState.PAID; + } + + _borrowerBidsActive[bid.borrower].remove(_bidId); + + + if (_shouldWithdrawCollateral) { + + collateralManager.withdraw(_bidId); + } + + emit LoanRepaid(_bidId); + } else { + emit LoanRepayment(_bidId); + } +@> _sendOrEscrowFunds(_bidId, _payment); // @audit-issue unmodified payment struct is passed, memory variable paymentAmount not used anywhere + } + +``` +The `paymentAmount >= _owedAmount` check is not performed correctly since `paymentAmount` is practically not used anywhere further down in the code. Unmodified `_payment` struct is passed in `_sendOrEscrowFunds` which triggers the [repayLoanCallback](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-L945) for the lender group incrementing `totalPrincipalTokensRepaid` by the amount in the `_payment` struct. + +Once `totalPrincipalTokensRepaid` is higher than `totalPrincipalTokensLended`, any attempts to issue loans will revert due to `getTotalPrincipalTokensOutstandingInActiveLoans` underflowing. Attack can be performed by backrunning commitment group creations and funding by taking a loan and repaying it immediately with an extra wei, bricking it forever since the underflowing function is used in `getMinInterestRate` which is called in `acceptFundsForAcceptBid` + +[Similar finding](https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/32) was part of the previous audit contest yet it was not fixed. Risk of DoS is still very much present and fix is rather simple. Furthermore the README states an invariant that should hold: +>Very importantly, we expect the following invariant: the 'EstimatedTotalValue' of a LenderGroupPool should accurately track the value of principal tokens in a pool. + +Even if a DoS is not performed, the value of principal tokens will not be tracked correctly since overpays will underestimate the free funds within the contract, lowering the utilization ratio too. + +### Root Cause + +- `_repayLoan` does not indicate whether a payment is the last one for a loan +- `sendOrEscrowFunds` performs a sanity check incorrectly and uses unmodified payment struct + +### Internal pre-conditions + +None + +### External pre-conditions + +None + +### Attack Path + +1. Lender commitment group is deployed and funded by owner +2. Adversary takes a loan for 100e6 USDC, immediately repays it through `repayLoan(bidId, 100e6+1) +3. Lender commitment group can no longer issue loans since `acceptFundsForAcceptBid` relies on `getTotalPrincipalTokensOutstandingInActiveLoans` which now underflows due to `100e6 - (100e6+1) < 0` + +### Impact + +DoS of entire group + +### PoC + +_No response_ + +### Mitigation + +If user invokes `repayLoan` with `amount` exceeding his total dues, perform `repayLoanFull` instead. \ No newline at end of file diff --git a/032.md b/032.md new file mode 100644 index 0000000..d94c859 --- /dev/null +++ b/032.md @@ -0,0 +1,98 @@ +Magic Rainbow Locust + +High + +# An Attacker Can Overwrite Storage Variables, Leading to Security Bypass and Potential Loss of User Funds + +### Summary + +An incorrect inheritance order in `SmartCommitmentForwarder.sol` will cause a critical vulnerability for users, as an attacker will be able to overwrite crucial storage variables due to storage slot collisions. This allows the attacker to bypass security mechanisms, such as the `nonReentrant` modifier, and manipulate the contract state in unintended ways, potentially leading to fund loss or other severe consequences. + + + +### Root Cause + +In `[SmartCommitmentForwarder.sol`](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/SmartCommitmentForwarder.sol#L51-L59), the incorrect order of inheritance leads to storage variables being overwritten due to overlapping storage slots. The contract inherits from `ReentrancyGuardUpgradeable`, `OwnableUpgradeable`, and `PausableUpgradeable` **after** introducing new storage variables, which causes the storage layout to be misaligned. + +```solidity +contract SmartCommitmentForwarder is + ExtensionsContextUpgradeable, // Should always be first for upgradeability + TellerV2MarketForwarder_G3, + PausableUpgradeable, // Adds storage variables but is inherited after other contracts + ReentrancyGuardUpgradeable, // Adds storage variables, potentially overwriting existing ones + OwnableUpgradeable, // Adds storage variables but is inherited after own variables + OracleProtectionManager, // Uses deterministic storage slots + ISmartCommitmentForwarder, + IPausableTimestamp +{ + // Contract body + uint256 public liquidationProtocolFeePercent; + uint256 internal lastUnpausedAt; + + // ... rest of the contract code ... +} +``` + +The storage variables from the base contracts (`ReentrancyGuardUpgradeable`, `OwnableUpgradeable`, and `PausableUpgradeable`) are inherited **after** the contract's own storage variables are declared, leading to storage slot collisions and overwriting essential variables. + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. **The attacker calls a function protected by the `nonReentrant` modifier:** + + - For example, the `acceptSmartCommitmentWithRecipient` function, which is marked as `nonReentrant`. + +2. **Due to storage collisions, the `nonReentrant` modifier does not function correctly:** + + - The `_status` variable from `ReentrancyGuardUpgradeable`, used to track reentrancy status, is overwritten or contains an incorrect value. + +3. **The attacker initiates a reentrant call into the contract:** + + - Because the `nonReentrant` protection is compromised, the attacker can recursively call the same function or another vulnerable function before the first call completes. + +4. **The attacker manipulates the contract's state or drains funds:** + + - The reentrancy allows the attacker to bypass logical checks, manipulate state variables, or withdraw funds multiple times. + +### Impact + +The users of the contract suffer a critical loss of funds as the contract's state is compromised, and security mechanisms are bypassed. This could result in theft of funds from the contract, disruption of expected behaviors, and loss of trust in the platform. + +- **Potential loss of all funds held by the contract.** +- **Bypassing of security mechanisms like the `nonReentrant` modifier.** +- **Ability for an attacker to manipulate essential state variables.** + +### PoC + +While a full PoC requires deployment and testing tools, the attack can be conceptually demonstrated: + +1. **Setup:** + + - The `SmartCommitmentForwarder` contract is deployed with the incorrect inheritance order. + - The attacker is aware of the storage collision vulnerability. + +2. **Attack Execution:** + + - The attacker calls the `acceptSmartCommitmentWithRecipient` function, which is marked as `nonReentrant`. + + - Due to the storage collision, the `_status` variable from `ReentrancyGuardUpgradeable` is overwritten and does not prevent reentrancy. + + - The attacker crafts the transaction to re-enter the `acceptSmartCommitmentWithRecipient` function before the first call completes. + +3. **Result:** + + - The attacker is able to bypass the `nonReentrant` modifier. + + - The attacker can manipulate contract state, such as increasing balances or withdrawing funds multiple times. + +### Mitigation + +_No response_ \ No newline at end of file diff --git a/033.md b/033.md new file mode 100644 index 0000000..05538ab --- /dev/null +++ b/033.md @@ -0,0 +1,71 @@ +Long Stone Urchin + +High + +# Incorrect Parameter Passed to `OracleProtectedChild` + +### Summary + +An incorrect parameter is passed to the `OracleProtectedChild` parent constructor in the `LenderCommitmentGroup_Smart` contract, as the **wrong address is provided for `_oracleManager`**, causing functionality relying on `OracleProtectedChild` to fail. + +### Root Cause + + +In the constructor of the `LenderCommitmentGroup_Smart` contract: + +```solidity +constructor( + address _tellerV2, + address _smartCommitmentForwarder, + address _uniswapV3Factory +) OracleProtectedChild(_smartCommitmentForwarder) { + TELLER_V2 = _tellerV2; + SMART_COMMITMENT_FORWARDER = _smartCommitmentForwarder; + UNISWAP_V3_FACTORY = _uniswapV3Factory; +} +``` +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L280 + +The `_smartCommitmentForwarder` is incorrectly passed as the parameter to the `OracleProtectedChild` parent constructor, but it should have been `_oracleManager`. This causes the `OracleProtectedChild` contract to be initialized with the wrong parameter. + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +The `OracleProtectedChild` contract is improperly initialized, functionality relying on `OracleProtectedChild` (e.g., `addPrincipalToCommitmentGroup`, `burnSharesToWithdrawEarnings` and `liquidateDefaultedLoanWithIncentive` could be exploited. + + +### PoC + +_No response_ + +### Mitigation + + +The parameter passed to the `OracleProtectedChild` constructor must be corrected. + +```diff + constructor( + address _tellerV2, + address _smartCommitmentForwarder, + address _uniswapV3Factory ++ address _oracleManager +- ) OracleProtectedChild(_smartCommitmentForwarder) { ++ ) OracleProtectedChild(_oracleManager) { + TELLER_V2 = _tellerV2; + SMART_COMMITMENT_FORWARDER = _smartCommitmentForwarder; + UNISWAP_V3_FACTORY = _uniswapV3Factory; + } +``` + diff --git a/034.md b/034.md new file mode 100644 index 0000000..6fd5e81 --- /dev/null +++ b/034.md @@ -0,0 +1,100 @@ +Bubbly Inky Sawfish + +Medium + +# Users can lower the interest rate by dividing a loan into multiple smaller loans + +### Summary +The `LenderCommitmentGroup_Smart.getMinInterestRate()` function calculates the minimum APR for borrowing. However, this APR is determined based on the utilization ratio, which includes the newly borrowed amount. This allows borrowers to reduce the APR by dividing a loan into multiple smaller loans. + +### Root Cause +The `LenderCommitmentGroup_Smart.getMinInterestRate()` function calculates the minimum APR for borrowing. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L1017-1023 +```solidity + function getMinInterestRate(uint256 amountDelta) public view returns (uint16) { + return interestRateLowerBound + + uint16( uint256(interestRateUpperBound-interestRateLowerBound) +@> .percent(getPoolUtilizationRatio(amountDelta ) + + ) ); + } +``` + +However, the APR is calculated based on the utilization ratio, which includes the newly borrowed amount. This allows borrowers to reduce the APR by dividing a loan into multiple smaller loans, creating an unfair advantage for them over other users. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L1002-1015 +```solidity + function getPoolUtilizationRatio(uint256 activeLoansAmountDelta ) public view returns (uint16) { + + if (getPoolTotalEstimatedValue() == 0) { + return 0; + } + + return uint16( Math.min( + MathUpgradeable.mulDiv( +@> (getTotalPrincipalTokensOutstandingInActiveLoans() + activeLoansAmountDelta), + 10000 , + getPoolTotalEstimatedValue() ) , + 10000 )); + + } +``` + + +### Internal pre-conditions +interestRateLowerBound = 0 +interestRateLowerBound = 800 // 8% +current total estimated value = $20000 +current borrowed value = $5000 +### External pre-conditions +none + +### Attack Path +If Alice borrows $10000 USD at once, the APR is (5000 + 10000) / 20000 * 8% = 6% + +However, Alice borrow $10000 as follows. +1. Alice is going to borrow $10000. +2. Alice borrows $1,000 repeatedly for a total of 10 times. + +Then the APR for each 1000 USD is: +(5000 + 1000) / 20000 * 8% = 2.4% +(6000 + 1000) / 20000 * 8% = 2.8% +(7000 + 1000) / 20000 * 8% = 3.2% +(8000 + 1000) / 20000 * 8% = 3.6% +(9000 + 1000) / 20000 * 8% = 4.0% +(10000 + 1000) / 20000 * 8% = 4.4% +(11000 + 1000) / 20000 * 8% = 4.8% +(12000 + 1000) / 20000 * 8% = 5.2% +(13000 + 1000) / 20000 * 8% = 5.6% +(14000 + 1000) / 20000 * 8% = 6% + +(2.4 + 2.8 + ... + 6) / 10 = 4.2 + +Therefore, the borrower will only pay an APR of 4.2% on the $10,000 loan. +As a result, Alice lowers the interest rate from 6% to 4.2% by splitting a $10,000 loan into ten separate loans of $1,000 each. + +### Impact +Users can lower the interest rate by dividing a loan into multiple smaller loans. + +### PoC +none + +### Mitigation +Middle value should be used instead of end value. + +```diff + function getPoolUtilizationRatio(uint256 activeLoansAmountDelta ) public view returns (uint16) { + + if (getPoolTotalEstimatedValue() == 0) { + return 0; + } + + return uint16( Math.min( + MathUpgradeable.mulDiv( +- (getTotalPrincipalTokensOutstandingInActiveLoans() + activeLoansAmountDelta), ++ (getTotalPrincipalTokensOutstandingInActiveLoans() + (activeLoansAmountDelta + 1) / 2), + 10000 , + getPoolTotalEstimatedValue() ) , + 10000 )); + + } +``` \ No newline at end of file diff --git a/035.md b/035.md new file mode 100644 index 0000000..7ff3cf8 --- /dev/null +++ b/035.md @@ -0,0 +1,85 @@ +Macho Taffy Owl + +High + +# Incorrect Parameter to OracleProtectedChild is pass during Constructor + +### Summary + +The constructor of the `LenderCommitmentGroup_Smart` contract incorrectly passes the `SMART_COMMITMENT_FORWARDER` address to the `OracleProtectedChild` contract instead of the `Oracle Manager` address, leading to potential authorization issues. + + +### Root Cause + + +In the constructor of the `LenderCommitmentGroup_Smart` contract, the following code: + +```solidity + /// @custom:oz-upgrades-unsafe-allow constructor + constructor( + address _tellerV2, + address _smartCommitmentForwarder, + address _uniswapV3Factory + @>> ) OracleProtectedChild(_smartCommitmentForwarder) { + TELLER_V2 = _tellerV2; + SMART_COMMITMENT_FORWARDER = _smartCommitmentForwarder; + UNISWAP_V3_FACTORY = _uniswapV3Factory; + } +``` + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L276C1-L284C6 + +incorrectly uses `_smartCommitmentForwarder` instead of the correct `Oracle Manager` address, leading to potential unauthorized access. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +These modifier we used in the `LenderCommitmentGroup_Smart` won't work because in `_smartCommitmentForwarder` address we don't have these isOracleApproved, isOracleApprovedAllowEOA functions and they will revert So call to addPrincipalToCommitmentGroup, burnSharesToWithdrawEarnings and liquidateDefaultedLoanWithIncentive will always revert because of onlyOracleApprovedAllowEOA modifier. + +```solidity + + modifier onlyOracleApproved() { + IOracleProtectionManager oracleManager = IOracleProtectionManager(ORACLE_MANAGER); + require( oracleManager .isOracleApproved(msg.sender ) , "Oracle: Not Approved"); + _; + } + + + modifier onlyOracleApprovedAllowEOA() { + IOracleProtectionManager oracleManager = IOracleProtectionManager(ORACLE_MANAGER); + require( oracleManager.isOracleApprovedAllowEOA(msg.sender) , "Oracle: Not Approved"); + _; + } +``` + +### PoC + +_No response_ + +### Mitigation + +Modify constructor to correctly pass the `ORACLE_MANAGER` address to the `OracleProtectedChild` contract. + +```solidity + + constructor( + address _tellerV2, + address _smartCommitmentForwarder, + address _uniswapV3Factory +@>> ) OracleProtectedChild(_smartCommitmentForwarder) { + TELLER_V2 = _tellerV2; + SMART_COMMITMENT_FORWARDER = _smartCommitmentForwarder; + UNISWAP_V3_FACTORY = _uniswapV3Factory; + } +``` \ No newline at end of file diff --git a/036.md b/036.md new file mode 100644 index 0000000..04d3082 --- /dev/null +++ b/036.md @@ -0,0 +1,39 @@ +Custom Pineapple Newt + +Medium + +# Users' prepared shares never expire + +### Summary + +Users in the smart commitment group can circumvent the time lock imposed for withdrawing assets. + +### Root Cause + +When users want to withdraw assets from the smart commitment group, they are first required to [prepare them for burning](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L593-L599). + +### Internal pre-conditions + +1. User must already own shares in the group + +### External pre-conditions + +None + +### Attack Path + +None + +### Impact + +A user can invest in the group with a large capital, then as soon as they receive their shares, prepare all of them for burning. This allows them to later circumvent the time lock and [redeem the assets instantly](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L612-L646). + +There is no expiration in place when preparing shares for burning which allows this. The user which has prepared their shares for burning in advance will be able to withdraw their assets instantly in case of emergency or unexpected market conditions while other users will have to wait out the time lock which can be as long as 24h. + +### PoC + +_No response_ + +### Mitigation + +If the preparation isn't redeemed in `t` amount of time, require a new one to be made. \ No newline at end of file diff --git a/037.md b/037.md new file mode 100644 index 0000000..554c4a3 --- /dev/null +++ b/037.md @@ -0,0 +1,57 @@ +Dandy Caramel Tortoise + +Medium + +# Attacker can revoke any user from a market + +### Summary + +Lack of access control in `revokeLender` allows an attacker to revoke any participant from a market + +### Root Cause + +The [delegation version](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L332-L338) of the `revokeLender` function fails to perform any access control checks allowing any user to revoke any user + +```solidity + function _revokeStakeholderViaDelegation( + uint256 _marketId, + address _stakeholderAddress, + bool _isLender, + uint8 _v, + bytes32 _r, + bytes32 _s + ) internal { + bytes32 uuid = _revokeStakeholderVerification( + _marketId, + _stakeholderAddress, + _isLender + ); + // NOTE: Disabling the call to revoke the attestation on EAS contracts + // address attestor = markets[_marketId].owner; + // tellerAS.revokeByDelegation(uuid, attestor, _v, _r, _s); + } +``` + +### Internal pre-conditions + +Attestation should be enabled to observe the impact + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Attacker calls `revokeLender` by passing in any address they wish to revoke from the market + +### Impact + +Attacker can revoke any address they wish from any market making the market unuseable + +### PoC + +_No response_ + +### Mitigation + +Perform access control checks \ No newline at end of file diff --git a/038.md b/038.md new file mode 100644 index 0000000..20a34d9 --- /dev/null +++ b/038.md @@ -0,0 +1,73 @@ +Dandy Caramel Tortoise + +Medium + +# Old market owner can grant access to users that new market owner will not be able to revoke + +### Summary + +Attesting by delegation will disallow new owner to revoke attestation of a stakeholder provided by earlier owner + +### Root Cause + +The `MarketRegistry` allows for stakeholders to be [registered via delegation](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L1078-L1086). For such an attestation the attestor recorded will be the market owner +```solidity + function _attestStakeholderViaDelegation( + uint256 _marketId, + address _stakeholderAddress, + uint256 _expirationTime, + bool _isLender, + uint8 _v, + bytes32 _r, + bytes32 _s + ) +``` + +To revoke the attestation properly (currently this is not handled in the MarketRegsitry contract), the `_revoke` function of the `TellerAS` contract has to be called. But here the person revoking should be the same as the attestor which will fail in case the owner of the market has been changed. Hence the new attestor cannot revoke a stakeholder +```solidity + function _revoke(bytes32 uuid, address attester) private { + Attestation storage attestation = _db[uuid]; + if (attestation.uuid == EMPTY_UUID) { + revert NotFound(); + } + + + if (attestation.attester != attester) { + revert AccessDenied(); + } + + + if (attestation.revocationTime != 0) { + revert AlreadyRevoked(); + } + + + attestation.revocationTime = block.timestamp; +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Owner attests a lender/borrower using delegation +2. The market is transferred to a new owner +3. The new owner cannot revoke the lender/borrower + +### Impact + +New owner cannot revoke a lender/borrower + + +### PoC + +_No response_ + +### Mitigation + +Disable the delegation feature or handle this in the TellerAS contract \ No newline at end of file diff --git a/039.md b/039.md new file mode 100644 index 0000000..e81b08c --- /dev/null +++ b/039.md @@ -0,0 +1,77 @@ +Dandy Caramel Tortoise + +High + +# Malicious lender can prevent borrower from repayment due to try/catch block revert + +### Summary + +Insufficient validation for `try/catch` address will disallow borrower's from repaying their loans + +### Root Cause + +A malicious lender can bypass the `try/catch` block covering the [repayLoanCallback](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L938-L948) external call by selfdestructing `loanRepaymentListener` + +```solidity + if (loanRepaymentListener != address(0)) { + require(gasleft() >= 80000, "NR gas"); //fixes the 63/64 remaining issue + try + ILoanRepaymentListener(loanRepaymentListener).repayLoanCallback{ + gas: 80000 + }( //limit gas costs to prevent lender preventing repayments + _bidId, + _msgSenderForMarket(bid.marketplaceId), + _payment.principal, + _payment.interest + ) +``` + +The `try/catch` block will revert if the call is made to a non-contract address. To avoid this a check for `codesize > 0` is kept inside the `setRepaymentListenerForBid` function. But this can be bypassed by the lender `selfdestructing` the `_listener` in the same [transaction which will delete the contract](https://eips.ethereum.org/EIPS/eip-6780) + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L1287-L1301 +```solidity + function setRepaymentListenerForBid(uint256 _bidId, address _listener) external { + uint256 codeSize; + assembly { + codeSize := extcodesize(_listener) + } + require(codeSize > 0, "Not a contract"); + address sender = _msgSenderForMarket(bids[_bidId].marketplaceId); + + + require( + sender == getLoanLender(_bidId), + "Not lender" + ); + + + repaymentListenerForBid[_bidId] = _listener; + } +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Lender creates a contract which can `selfdestruct` itself +2. Lender sets this address as the repaymentListener +3. In the same tx, the lender destroys the contract +4. Now the borrower cannot repay because the try/catch block will always revert + +### Impact + +Borrowers will not be able to repay the loan allowing the lender to steal the collateral after the loan will default + +### PoC + +_No response_ + +### Mitigation + +Use .call instead of the try/catch \ No newline at end of file diff --git a/040.md b/040.md new file mode 100644 index 0000000..f8266ad --- /dev/null +++ b/040.md @@ -0,0 +1,50 @@ +Dandy Caramel Tortoise + +Medium + +# Loan repayment can be intentionally done with slightly less than 80k gas + +### Summary + +Incorrect check will cause the `loanRepaymentListener` to receive less than 80k gas limit + +### Root Cause + +The [check performed](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L938-L945) before making the call to `loanRepaymentListener` is that there is atleast 80k gasleft + +```solidity + if (loanRepaymentListener != address(0)) { + require(gasleft() >= 80000, "NR gas"); //fixes the 63/64 remaining issue + try + ILoanRepaymentListener(loanRepaymentListener).repayLoanCallback{ + gas: 80000 + }( //limit gas costs to prevent lender preventing repayments + _bidId, + _msgSenderForMarket(bid.marketplaceId), +``` + +But this will fail to send 80k gas to the `loanRepaymentListener` due to the 63/64 rule ie. if 81250 gas is present, then only (81250 * 63/64 == 79980) gas will be sent. If the `loanRepaymentListener` expects exactly 80k gas for its execution, it will cause its execution to revert + +### Internal pre-conditions + +`loanRepaymentListener` should need very close to 80k gas for its execution + +### External pre-conditions + +_No response_ + +### Attack Path + +1. The loan is repaid with a gas amount such that the gas sent to `loanRepaymentListener` is less than 80k + +### Impact + +Attacker can intentionally make the execution of `loanRepaymentListener` revert + +### PoC + +_No response_ + +### Mitigation + +Keep a buffer and not the exact amount so that there is exactly 80k gas to be sent to the `loanRepaymentListener` \ No newline at end of file diff --git a/041.md b/041.md new file mode 100644 index 0000000..5840fba --- /dev/null +++ b/041.md @@ -0,0 +1,66 @@ +Dandy Caramel Tortoise + +Medium + +# Failure to use updated payment amount will cause repayer's to loose assets in case they pay in excess + +### Summary + +Failure to use updated payment amount will cause re-payer's to loose assets in case they pay in excess + +### Root Cause + +In case the payment is greater than the owed amount, the `_sendOrEscrowFunds` function still uses the excess amount instead of the owed amount. This causes the excess amounts to be lost to the borrower + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L828-L844 +```solidity + function _repayLoan( + uint256 _bidId, + Payment memory _payment, + uint256 _owedAmount, + bool _shouldWithdrawCollateral + ) internal virtual { + Bid storage bid = bids[_bidId]; + uint256 paymentAmount = _payment.principal + _payment.interest; + + + RepMark mark = reputationManager.updateAccountReputation( + bid.borrower, + _bidId + ); + + + // Check if we are sending a payment or amount remaining + if (paymentAmount >= _owedAmount) { + paymentAmount = _owedAmount; + + ... + + // @audit the old _payment amount is still used instead of the updated paymentAmount + _sendOrEscrowFunds(_bidId, _payment); //send or escrow the funds +``` + + +### Internal pre-conditions + +1. User must make excess repayment + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Borrowers will loose the excess they have paid + +### PoC + +_No response_ + +### Mitigation + +Cap the payment to `amountOwed` \ No newline at end of file diff --git a/042.md b/042.md new file mode 100644 index 0000000..93d7a13 --- /dev/null +++ b/042.md @@ -0,0 +1,85 @@ +Dandy Caramel Tortoise + +Medium + +# Not updating state before making custom external call can cause borrower's to loose assets due to re-entrancy + +### Summary + +Not updating state before making custom external call can cause borrower's to loose assets due to re-entrancy + +### Root Cause + +The details of the repayment is updated only after the external call to the `loanRepaymentListener` is made + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L865-L870 +```solidity + function _repayLoan( + uint256 _bidId, + Payment memory _payment, + uint256 _owedAmount, + bool _shouldWithdrawCollateral + ) internal virtual { + + .... + // @audit attacker can re-enter here. the repayment details are not yet updated + _sendOrEscrowFunds(_bidId, _payment); //send or escrow the funds + + // update our mappings + bid.loanDetails.totalRepaid.principal += _payment.principal; + bid.loanDetails.totalRepaid.interest += _payment.interest; + bid.loanDetails.lastRepaidTimestamp = uint32(block.timestamp); +``` + +```solidity + function _sendOrEscrowFunds(uint256 _bidId, Payment memory _payment) + internal virtual + { + + .... + + address loanRepaymentListener = repaymentListenerForBid[_bidId]; + + // @audit re-enter in this call + if (loanRepaymentListener != address(0)) { + require(gasleft() >= 80000, "NR gas"); //fixes the 63/64 remaining issue + try + ILoanRepaymentListener(loanRepaymentListener).repayLoanCallback{ + gas: 80000 + }( //limit gas costs to prevent lender preventing repayments + _bidId, + _msgSenderForMarket(bid.marketplaceId), + _payment.principal, + _payment.interest + ) + {} catch {} +``` + +This allows a malicious lender to reenter the `TellerV2` contract and invoke `lenderCloseLoan` seizing the collateral of the borrower as well if the loan is currently defaulted + +### Internal pre-conditions + +1. The repayment should be made after defaultTimestamp has passed + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Defaulting timestmap of loan has passed +2. Borrower does a repayment of 100 which is transferred to the lender. Following this `.repayLoanCallback` is called +3. Lender reenters via the `loanRepaymentListener` and invokes the `lenderCloseLoan` function further seizing the collateral of the borrower +4. Borrower looses both the repayment amount and the collateral + +### Impact + +Borrower will loose repayment amount and also the collateral + +### PoC + +_No response_ + +### Mitigation + +Update the state before the `loanRepaymentListener` call is made \ No newline at end of file diff --git a/043.md b/043.md new file mode 100644 index 0000000..e0d221b --- /dev/null +++ b/043.md @@ -0,0 +1,108 @@ +Dandy Caramel Tortoise + +High + +# Using original principal amount as due amount inside `liquidateDefaultedLoanWithIncentive` breaks contract accounting leading to lost assets/broken functionalities + +### Summary + +Using original principal amount as due amount inside `liquidateDefaultedLoanWithIncentive` breaks contract accounting leading to lost assets/broken functionalities + +### Root Cause + +In `liquidateDefaultedLoanWithIncentive`, the amount due is taken as the [original principal amount](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L657-L664) of the bid rather than the remaining to be repaid principal which is incorrect as part of this principal could [have already been paid back](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-L935) + +```solidity + function liquidateDefaultedLoanWithIncentive( + uint256 _bidId, + int256 _tokenAmountDifference + ) external whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) nonReentrant onlyOracleApprovedAllowEOA { + + //use original principal amount as amountDue + + uint256 amountDue = _getAmountOwedForBid(_bidId); +``` + +```solidity + function _getAmountOwedForBid(uint256 _bidId ) + internal + view + virtual + returns (uint256 amountDue) + { + // @audit this is the entire principal amount which is incorrect + (,,,, amountDue, , , ) + = ITellerV2(TELLER_V2).getLoanSummary(_bidId); + } +``` + +Parts of the original principal could have been repaid and accounted via this function leading to double counting of principal in `totalPrincipalTokensRepaid` +```solidity + function repayLoanCallback( + uint256 _bidId, + address repayer, + uint256 principalAmount, + uint256 interestAmount + ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { + totalPrincipalTokensRepaid += principalAmount; + totalInterestCollected += interestAmount; +``` + +This leads to several problems: +1. Underflow in `totalPrincipalTokensLended - totalPrincipalTokensRepaid` as `totalPrincipalTokensRepaid` can double count repaid tokens causing bids to revert +2. Lost assets due to `tokenDifferenceFromLiquidations` calculating difference from `totalPrincipal` without considering the repaid assets + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. A loan is created with principal amount == 100 +tokenBalanceOfContract == 0 + +totalPrincipalTokensCommitted == 100 +totalPrincipalTokensWithdrawn == 0 +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 0 +tokenDifferenceFromLiquidations == 0 + +2. Repayment of 80 principal occurs before the loan gets defaulted +tokenBalanceOfContract == 80 + +totalPrincipalTokensCommitted == 100 +totalPrincipalTokensWithdrawn == 0 +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 80 +tokenDifferenceFromLiquidations == 0 + +3. Loan defaults and auction settles at price 50 (similarly problematic paths are lenders withdrawing 80 first or the auction settling at higher prices) +tokenBalanceOfContract == 80 + 50 == 130 + +totalPrincipalTokensCommitted == 100 +totalPrincipalTokensWithdrawn == 0 +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 80 + 100 == 180 => incorrect +tokenDifferenceFromLiquidations == (100 - 50 == -50) => incorrect + +Now: +- available amount to withdraw will be calculated as (totalPrincipalTokensCommitted + tokenDifferenceFromLiquidations == 50) while there is actually 130 amount of assets available to withdraw causing loss for lenders +- getPrincipalAmountAvailableToBorrow will underflow because (totalPrincipalTokensLended - totalPrincipalTokensRepaid == -80) and no new bids can be accepted + +There are more scenarios that arise from the same root cause such as estimated value becoming 0 incorrectly, which will cause division by 0 and hence revert on withdrawals, deposits will be lost or 0 shares will be minted etc. + +### Impact + +Lost assets for users, broken functionalities + +### PoC + +_No response_ + +### Mitigation + +Instead of the totalPrincipal consider the remaining principal ie. `totalPrincipal - repaidPrincipal` \ No newline at end of file diff --git a/044.md b/044.md new file mode 100644 index 0000000..51b8d5a --- /dev/null +++ b/044.md @@ -0,0 +1,41 @@ +Dandy Caramel Tortoise + +Medium + +# Using `.approve` will cause bidding to revert in LenderCommitmentGroup_Smart.sol + +### Summary + +Using `.approve` will cause bidding to revert in LenderCommitmentGroup_Smart.sol + +### Root Cause + +`.approve` is [used](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L556) expecting a boolean return variable which will revert for USDT since it doesn't return anything. The team specifically plans to support USDT +```solidity + principalToken.approve(address(TELLER_V2), _principalAmount); +``` + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Bidding in `LenderCommitmentGroup_Smart` will revert for USDT + +### PoC + +_No response_ + +### Mitigation + +Use `.safeApprove` instead \ No newline at end of file diff --git a/045.md b/045.md new file mode 100644 index 0000000..41f81e0 --- /dev/null +++ b/045.md @@ -0,0 +1,51 @@ +Dandy Caramel Tortoise + +Medium + +# Borrowers can grief lenders due to lack of minimum bid amount in `LenderCommitmentGroup_Smart` + +### Summary + +Lack of `minimum` debt requirements can cause lenders loose assets to griefing attacks + +### Root Cause + +There are [no `minimum debt amount` checks](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L518-L527) enforced inside `acceptFundsForAcceptBid`. As a result an attacker can take out loans of very small amounts which will be not profitable (gas costwise) to liquidate in the open market or until significant losses is incurred by the lender contract. This will cause the lenders to loose their deposited assets + +```solidity + function acceptFundsForAcceptBid( + address _borrower, + uint256 _bidId, + uint256 _principalAmount, + uint256 _collateralAmount, + address _collateralTokenAddress, + uint256 _collateralTokenId, + uint32 _loanDuration, + uint16 _interestRate + ) external onlySmartCommitmentForwarder whenForwarderNotPaused whenNotPaused { +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Lender deposits 1000 into `LenderCommitmentGroup_Smart` +2. Attacker creates loans of (10 * 100). The collateral backing each loan of 10 is assumed to be of 15 in value. This amount is not profitable to be liquidated in the open market due to gas costs until the amount the liquidators have to pay drops far below 10 causing lost assets for the lenders + +### Impact + +Lost assets for the lenders + +### PoC + +_No response_ + +### Mitigation + +Enforce a minimum amount requirement for the borrowing \ No newline at end of file diff --git a/046.md b/046.md new file mode 100644 index 0000000..7ab24da --- /dev/null +++ b/046.md @@ -0,0 +1,81 @@ +Dandy Caramel Tortoise + +High + +# Repayer can brick lending functionality of `LenderCommitmentGroup_Smart` by repaying excess + +### Summary + +`LenderCommitmentGroup_Smart` doesn't handle excess repayments making it possible to brick the lending functionality + +### Root Cause + +The [`getTotalPrincipalTokensOutstandingInActiveLoans` function](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L965-L972) performs `totalPrincipalTokensLended - totalPrincipalTokensRepaid` without handling the underflow scenario. This is problematic as excess amount can be repaid for loans which will cause an underflow here + +```solidity + function getTotalPrincipalTokensOutstandingInActiveLoans() + public + view + returns (uint256) + { + return totalPrincipalTokensLended - totalPrincipalTokensRepaid; + } +``` + +on excess repayments, `totalPrincipalTokensRepaid` will become greater than `totalPrincipalTokensLended` +```solidity + function repayLoanCallback( + uint256 _bidId, + address repayer, + uint256 principalAmount, + uint256 interestAmount + ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { + totalPrincipalTokensRepaid += principalAmount; + totalInterestCollected += interestAmount; +``` + +function in TellerV2.sol contract to repay excess amount. User can specify any amount greater than minimum amount +```solidity + function repayLoan(uint256 _bidId, uint256 _amount) + external + acceptedLoan(_bidId, "rl") + { + _repayLoanAtleastMinimum(_bidId, _amount, true); + } +``` + +The function `getTotalPrincipalTokensOutstandingInActiveLoans` is invoked before every lending. Hence an underflow here will cause the lending functionality to revert + +Another quirk regarding excess repayments is that the lenders of the pool won't obtain the excess repaid amount since it is not accounted anywhere. But this cannot be considered an issue since the lenders are only guarenteed the original lending amount + interest + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Attacker borrows 100 from the lender pool +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 0 + +2. Attacker repays 101 +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 101 + +Now `getTotalPrincipalTokensOutstandingInActiveLoans` will always revert + +### Impact + +Bricked lending functionality + +### PoC + +_No response_ + +### Mitigation + +In case repaid principal is more, return 0 instead \ No newline at end of file diff --git a/047.md b/047.md new file mode 100644 index 0000000..41d42ba --- /dev/null +++ b/047.md @@ -0,0 +1,89 @@ +Dandy Caramel Tortoise + +Medium + +# Attacker can DOS the `by delegation methods` of the market registry contract by directly passing the signature to Verifier contract + +### Summary + +Attacker can DOS the `by delegation methods` of the market registry contract by directly passing the signature to Verifier contract + +### Root Cause + +The `MarketRegistry` contract exposes attestaion/revocation [methods based on delegation ie. signatures](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L1078-L1087). These signatures are then passed on to the `TellerASEIP712Verifier` contract for verification + +```solidity + function _attestStakeholderViaDelegation( + uint256 _marketId, + address _stakeholderAddress, + uint256 _expirationTime, + bool _isLender, + uint8 _v, + bytes32 _r, + bytes32 _s + ) + internal +``` + +But the [Verifier contract allows anybody to call it and increments the nonce if a valid signature is passed](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/EAS/TellerASEIP712Verifier.sol#L68-L100). This allows a user to pass the delegation signature directly to the verifier contract which will increment the nonce of the signer hence reverting when the actual call from the MarketRegistry happens +```solidity + function attest( + address recipient, + bytes32 schema, + uint256 expirationTime, + bytes32 refUUID, + bytes calldata data, + address attester, + uint8 v, + bytes32 r, + bytes32 s + ) external override { + bytes32 digest = keccak256( + abi.encodePacked( + "\x19\x01", + DOMAIN_SEPARATOR, + keccak256( + abi.encode( + ATTEST_TYPEHASH, + recipient, + schema, + expirationTime, + refUUID, + keccak256(data), + _nonces[attester]++ + ) + ) + ) + ); + + address recoveredAddress = ecrecover(digest, v, r, s); + if (recoveredAddress == address(0) || recoveredAddress != attester) { + revert InvalidSignature(); + } +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. MarketRegistry admin decides to attest a user by delegation +2. Attacker frontruns this tx and submits the signature to the Verifier contract direclty +3. The call from MarketRegistry reverts because the nonce has already been increased and the signature wouldn't match + +### Impact + +Attacker can DOS the byDelegation methods of attestation and revocation (currently revoking doesn't pass on the call to TellerAS contract but ideally it would) + +### PoC + +_No response_ + +### Mitigation + +Maintain an internal verification mechanism rather than doing it in an external contract or enforce access control \ No newline at end of file diff --git a/048.md b/048.md new file mode 100644 index 0000000..19a960a --- /dev/null +++ b/048.md @@ -0,0 +1,206 @@ +Wild Chili Nuthatch + +High + +# Unchecked External Call (Collateral Transfer / Escrow) Risk + +### Summary + +[https://github.com/teller-protocol/teller-protocol-v2-audit-2024/blob/5226a72da9510d5676970386f4def6aa34d52fcc/packages/contracts/contracts/CollateralManager.sol#L349-L354](url) + +The contract makes external calls to transfer collateral tokens or interact with the escrow contract. However, these external calls are not properly checked for success, which introduces a risk where the transaction could silently fail without proper error handling. This can lead to unexpected behavior, such as the loss of collateral or funds, if the external contract does not behave as expected. + +### Root Cause + +The root cause lies in the reliance on external contract interactions (via the onlyTellerV2 modifier) without adequately validating the inputs and outputs of the repayLoanCallback function. Specifically: + +Implicit Trust in onlyTellerV2: + +The onlyTellerV2 modifier assumes that the designated TellerV2 contract is both correct and secure. +If the address used by the modifier points to a malicious or compromised contract, the protocol's logic can be manipulated. +Lack of Input Validation: + +The function does not verify the principalAmount and interestAmount values against actual loan data in the system. +This allows unchecked, externally provided values to directly modify critical state variables (totalPrincipalTokensRepaid and totalInterestCollected). +No Cross-Validation Against Loan Data: + +The function does not fetch or compare the repayment data with a trusted source (e.g., a loan registry or internal mapping). +Malicious actors could exploit this by calling the function with inflated or arbitrary values. +State Update Without Verification: + +The function directly updates state variables (totalPrincipalTokensRepaid and totalInterestCollected) based on unverified input, creating a risk of incorrect protocol state. + +### Internal pre-conditions + +Here is a numbered list of conditions that can allow the attack path or vulnerability path to happen: + +ProtocolOwner needs to set collateralEscrowBeacon to `attacker's contract address**. + +This allows an attacker to control the collateral escrow contract, enabling the manipulation of collateral transfers. +LenderGroupPool Owner needs to set principalPerCollateral to 0 within the pool's settings, relying on an external oracle's price. + +This introduces reliance on an external price oracle, which could be manipulated, allowing the attacker to influence loan terms or collateralization ratios. +CollateralManager needs to set bidCollateral to collateral that is not validated within the escrow setup process. + +If invalid collateral types are set without validation, this can lead to collateral acceptance from malicious tokens that may not meet the intended criteria. +Contract Owner or Pauser Role needs to set isPaused to true within the contract for liquidation actions. + +This halts the liquidation mechanism, preventing lenders from retrieving funds, which can be exploited during periods of market instability. +Escrow contract needs to accept invalid tokens for collateral within the escrow transfer process. + +The lack of proper validation allows attackers to deposit unauthorized tokens, which could block liquidation or lead to incorrect collateralization. +CollateralManager needs to execute external calls to transfer collateral without checking for failures. + +If the external contract fails without proper error handling, the transaction may silently fail, potentially causing loss of collateral or funds. +Lender needs to withdraw shares or collateral before the auction ends within a given liquidation period. + +If the timing of withdrawals is manipulated, it could result in inaccurate liquidation outcomes, impacting the pool's liquidity and fairness in the liquidation process. +These conditions form a chain that, if exploited, could lead to various vulnerabilities such as mismanagement of collateral, paused contracts affecting operations, or loss of funds due to external failures or attacks on the oracle system. + +### External pre-conditions + +Here are some external pre-conditions that could enable the attack path or vulnerability path to occur: + +Price Oracle needs to go from 500 to 1000 within 5 minutes. + +A sudden shift in the price of collateral or principal tokens might lead to manipulation of the principalPerCollateral ratio, allowing malicious actors to undercollateralize loans or impact the liquidation process. +Gas price needs to be exactly 100 gwei. + +A higher-than-normal gas price could force certain transactions (such as liquidations or collateral transfers) to be executed faster, giving attackers a window of opportunity to exploit the system for profit. +Uniswap liquidity pool needs to be manipulated, where the slippage tolerance is set too high, allowing for flash loan attacks to manipulate prices and exploit the collateral-to-loan ratio. + +This could affect the validity of the price used by the smart contract (e.g., TWAP prices) and allow attackers to benefit from liquidating underpriced collateral. +Collateral token's ERC20 transfer function needs to fail silently, without a revert or exception. + +If a token used as collateral does not revert on failure, this could allow the contract to accept invalid collateral, potentially exploiting the loan process or leading to funds being locked in the contract. +Attacker-controlled contract needs to be deployed to a known address and interact with the escrow beacon address, substituting a benign contract with a malicious one. + +This could allow the attacker to control the collateral handling process, including the acceptance of unauthorized tokens, which could then block liquidations or cause mismanagement of the funds. +Unusual price spikes or drops need to occur on the collateral token's market, impacting the principalPerCollateral ratio or loan parameters. + +This might lead to undercollateralized loans being issued or liquidation thresholds not being triggered in time, causing protocol malfunctions. +ERC721/1155 token standards need to be improperly validated by external contracts, allowing the use of invalid tokens as collateral for loans. + +If tokens are not properly validated by the external collateral manager or escrow contract, this could lead to unauthorized collateral being accepted and used in the loan system. +These external pre-conditions could create vulnerabilities that attackers can exploit if they occur in a specific sequence or under certain conditions. + +### Attack Path + +Here is an attack path for exploiting a vulnerability in the system: + +Attacker deploys a malicious contract: + +The attacker deploys a contract that is capable of interacting with the system, specifically targeting the collateral management or loan creation process. This contract could be designed to interact with vulnerable functions in the CollateralManager or related contracts. +Attacker manipulates collateral token's transfer behavior: + +The malicious contract could manipulate the behavior of a collateral token, such as an ERC20 token, to cause it to fail silently or to behave in an unexpected way. The attacker could create or exploit an ERC20 token where the transfer function does not revert on failure, allowing them to send invalid tokens or bypass checks in the contract. +Attacker calls the loan creation function: + +The attacker calls a function to request a loan on the protocol, supplying the manipulated or invalid collateral token. They may also set up the collateral amount and other parameters (such as the interest rate and loan duration) to ensure the loan is accepted. +Collateral validation bypassed: + +The malicious contract submits the loan request with the manipulated collateral. The CollateralManager contract or other collateral validation checks might not properly identify the invalid collateral due to improper ERC20/ERC721/ERC1155 validation, causing the loan to be processed without the proper collateral being checked. +Protocol issues an undercollateralized loan: + +The protocol issues a loan that is undercollateralized, allowing the attacker to receive funds or leverage the protocol’s liquidity without offering the required amount of collateral. This is a result of the invalid collateral being accepted, which could otherwise lead to the loss of funds for lenders. +Collateral is not correctly escrowed: + +In the case of ERC721 or ERC1155 tokens, the attacker might exploit flaws in the escrow contract (e.g., improper handling or validation) to move collateral out of escrow or bypass checks, further enriching themselves while the loan remains outstanding. +Liquidation vulnerability exposed: + +If the loan becomes overdue or requires liquidation, the protocol might fail to properly liquidate the undercollateralized loan due to incorrect collateral handling. This failure could occur because the collateral does not meet the expected type or value, or because it was improperly escrowed or transferred. +Attacker benefits from failed liquidations or collateral manipulation: + +The attacker benefits by either receiving unauthorized funds from the loan or by manipulating the liquidation process to their advantage. This could involve extracting more funds than their collateral would allow or blocking liquidations altogether. +Profitable exploitation: + +Through these steps, the attacker is able to exploit the vulnerability, profiting by taking out loans with insufficient collateral, preventing proper liquidation, and potentially draining the system of funds without repercussion. +By manipulating the collateral validation process, interacting with vulnerable escrow contracts, and bypassing checks, the attacker is able to exploit the protocol for their own gain. + +### Impact + +Attack Path Impact: +The lenders suffer an approximate loss of collateral value due to undercollateralized loans being issued by the attacker exploiting the collateral validation flaw. The attacker gains this collateral value or profits from loans they shouldn't have been able to obtain. +Vulnerability Path Impact: +The users suffer an approximate loss of 0.01% due to precision loss in calculations related to the loan collateral or interest rate, leading to a slight but non-negligible financial disadvantage for users. + +### PoC + +PoC for Attack Path (Undercollateralized Loans) +Scenario: +Vulnerability: A flaw in the collateral validation allows an attacker to create an undercollateralized loan, exploiting the system to borrow more funds than they are entitled to. +Impact: The attacker exploits this flaw to siphon funds from the lending pool by undercollateralizing loans, causing lenders to suffer a loss. +Proof of Concept: +```solidity + +// Assuming the collateral validation function is flawed and doesn't check collateral accurately. +contract MaliciousLender { + ICollateralManager public collateralManager; + ITellerV2 public tellerV2; + + constructor(address _collateralManager, address _tellerV2) { + collateralManager = ICollateralManager(_collateralManager); + tellerV2 = ITellerV2(_tellerV2); + } + + function exploitUndercollateralizedLoan(address borrower, uint256 bidId) external { + // Attacker calls a function with the invalid collateral, bypassing the collateral checks + // This results in the system approving an undercollateralized loan. + collateralManager.submitCollateral(bidId, 0, address(0), 0, address(this)); // Exploit method with invalid collateral + + // Submit a loan with faulty collateral to the teller contract + tellerV2.submitLoanRequest(borrower, bidId, 1000, 500); // Borrow 1000 with 500 collateral (under-collateralized) + } +} +``` + +Execution: +The attacker deploys a contract and exploits the flaw by calling the submitCollateral function with faulty collateral details. +The attacker submits a loan request that should not be approved, as it is undercollateralized. +The tellerV2 contract mistakenly approves the loan, allowing the attacker to borrow more than they should. +Outcome: +The lenders suffer a financial loss due to the undercollateralized loans being approved. +The attacker gains the over-borrowed amount of funds. +PoC for Vulnerability Path (Precision Loss) +Scenario: +Vulnerability: Precision loss during calculation of loan interest or collateral value results in small but constant discrepancies. +Impact: Users lose a tiny amount of funds due to these precision issues in calculations. +Proof of Concept: +```solidity + +contract LoanWithPrecisionLoss { + // Assume this contract deals with ERC20 tokens for collateral + IERC20 public collateralToken; + uint256 public interestRate; + + constructor(address _collateralToken, uint256 _interestRate) { + collateralToken = IERC20(_collateralToken); + interestRate = _interestRate; + } + + function calculateLoanAmount(uint256 principal) external view returns (uint256) { + // Assume this calculation is prone to precision loss due to lack of proper scaling + uint256 interest = principal * interestRate / 10000; // Risk of precision loss + return principal + interest; + } + + function takeLoan(uint256 principal) external { + uint256 totalAmount = calculateLoanAmount(principal); + + // Send the calculated amount to the borrower (leaving out proper scaling fixes) + collateralToken.transfer(msg.sender, totalAmount); + } +} +``` + +Execution: +The user calls the takeLoan function with a principal amount, and the contract calculates the interest using faulty precision. +Due to the lack of proper scaling (e.g., missing multiplication with a factor like 10^18), the contract experiences precision loss. +The user receives slightly less than expected, resulting in a tiny financial loss over time. +Outcome: +The user suffers a small, continuous loss due to precision errors in the contract's interest calculation. +Both of these PoCs demonstrate how the respective vulnerabilities can be exploited, resulting in financial loss or incorrect behavior in the system. The attack path involves exploiting faulty validation, while the vulnerability path deals with a precision issue in calculations. + +### Mitigation + +Implementing these mitigations—such as reentrancy guards, careful validation, safe transfer practices, and dedicated escrow contracts—can significantly reduce the risks associated with unchecked external calls in CollateralManager.sol. Adopting OpenZeppelin libraries and emitting logs will also improve security and traceability. \ No newline at end of file diff --git a/049.md b/049.md new file mode 100644 index 0000000..1e7fe4b --- /dev/null +++ b/049.md @@ -0,0 +1,94 @@ +Dandy Caramel Tortoise + +Medium + +# ERC-777 reentrancy will allow an attacker to permenantly cause some assets to be non-loanable in a LenderGroup + +### Summary + +ERC-777 reentrancy will allow an attacker to permenantly cause some assets to be non-loanable in a LenderGroup + +### Root Cause + +In the [readme the team has mentioned](https://github.com/sherlock-audit/2024-11-teller-finance-update/tree/main?tab=readme-ov-file#q-if-you-are-integrating-tokens-are-you-allowing-only-whitelisted-tokens-to-work-with-the-codebase-or-any-complying-with-the-standard-are-they-assumed-to-have-certain-properties-eg-be-non-reentrant-are-there-any-types-of-weird-tokens-you-want-to-integrate) that they would like to know if any wierd token breaks their contract pools. + +The `repaymentListener` is [set after the loan is accepted](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L581-L588) + +```solidity + function _acceptBidWithRepaymentListener(uint256 _bidId) internal { + ITellerV2(TELLER_V2).lenderAcceptBid(_bidId); //this gives out the funds to the borrower + + // @audit repayment listener is set after the ERC777 token is transferred from the borrower + ILoanRepaymentCallbacks(TELLER_V2).setRepaymentListenerForBid( + _bidId, + address(this) + ); +``` + +In case an ERC777 token is used as collateral/principal, the borrower can re-enter [during the token deposit](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L217)/token receival, and repay the loan + +```solidity + function deployAndDeposit(uint256 _bidId) external onlyTellerV2 { + if (isBidCollateralBacked(_bidId)) { + (address proxyAddress, ) = _deployEscrow(_bidId); + _escrows[_bidId] = proxyAddress; + + + for ( + uint256 i; + i < _bidCollaterals[_bidId].collateralAddresses.length(); + i++ + ) { + // @audit re-enter here and repay loan + _deposit( + _bidId, + _bidCollaterals[_bidId].collateralInfo[ + _bidCollaterals[_bidId].collateralAddresses.at(i) + ] +``` + +When a loan is repaid, the [repayLoanCallback](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-L936) function is supposed to be invoked in-order to increment the `totalPrincipalTokensRepaid` failure to do which will result in that much tokens forever being considered as `still lended` +```solidity + function repayLoanCallback( + uint256 _bidId, + address repayer, + uint256 principalAmount, + uint256 interestAmount + ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { + totalPrincipalTokensRepaid += principalAmount; + totalInterestCollected += interestAmount; +``` + +Since in this case repayment listener has not been setup, the `totalPrincipalTokensRepaid` won't be incremented. This will cause these tokens to forever be non-lendable + +### Internal pre-conditions + +1. ERC777 should be used as a collateral + +### External pre-conditions + +_No response_ + +### Attack Path + +0. Pool is created with an ERC777 collateral and has 100 tokens balance +1. Attacker calls `acceptSmartCommitmentWithRecipient` in the SmartCommitmentForwarder with 100 as the bid amount +2. Attacker re-enters when transferring the token and invokes `repayLoanFullWithoutCollateralWithdraw` + +Net result: +totalPrincipalTokensLended == 100 +totalPrincipalTokensRepaid == 0 + +Even though 100 amount has been repaid, causing this amount to be forever non-lendable + +### Impact + +Attacker can cause assets of pool to be non-lendable + +### PoC + +_No response_ + +### Mitigation + +Add nonReentrant modifier in repay functions and lenderAcceptBid function \ No newline at end of file diff --git a/050.md b/050.md new file mode 100644 index 0000000..9f82945 --- /dev/null +++ b/050.md @@ -0,0 +1,72 @@ +Dandy Caramel Tortoise + +Medium + +# Lack of sequencer uptime check can cause lenders to loose assets in L2 + +### Summary + +Lack of sequencer uptime check can cause the dutch auction for collateral to be settled at very low prices making the lenders loose their assets + +### Root Cause + +The [`liquidateDefaultedLoanWithIncentive` function](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L657-L660) performs a Dutch auction in case of defaulted loans +```solidity + function liquidateDefaultedLoanWithIncentive( + uint256 _bidId, + int256 _tokenAmountDifference + ) external whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) nonReentrant onlyOracleApprovedAllowEOA { +``` + +```solidity + * @dev As time approaches infinite, the output approaches -1 * AmountDue . + */ + function getMinimumAmountDifferenceToCloseDefaultedLoan( + uint256 _amountOwed, + uint256 _loanDefaultedTimestamp + ) public view virtual returns (int256 amountDifference_) { + require( + _loanDefaultedTimestamp > 0, + "Loan defaulted timestamp must be greater than zero" + ); + require( + block.timestamp > _loanDefaultedTimestamp, + "Loan defaulted timestamp must be in the past" + ); + + + uint256 secondsSinceDefaulted = block.timestamp - + _loanDefaultedTimestamp; + + + //this starts at 764% and falls to -100% + int256 incentiveMultiplier = int256(86400 - 10000) - + int256(secondsSinceDefaulted); + +``` + +And the team plans to launch on L2's like Arbitrum, Base etc which can have sequencer outages. In case the sequencer is down, liquidations won't happen until the sequencer comes back or the force inclusion delay passes. During this time the auction's price will have decreased significantly causing the auction to be settled at a very low price leading to losses for the lenders. Currently the price goes from (8.64x amountDue to 0 in 1 day) and delays in matter of hours can have significant impact on the price considering the lower range of price spectrum is the most likely one + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +1. Sequencer outage + +### Attack Path + +_No response_ + +### Impact + +Lenders will loose assets due to unfair auction + +### PoC + +_No response_ + +### Mitigation + +Check for sequencer uptime similar to paused time \ No newline at end of file diff --git a/051.md b/051.md new file mode 100644 index 0000000..04769cf --- /dev/null +++ b/051.md @@ -0,0 +1,72 @@ +Dandy Caramel Tortoise + +Medium + +# Tokens that revert of zero value transfers can cause reverts on liquidation + +### Summary + +Tokens that revert of zero value transfers can cause reverts on liquidation + +### Root Cause + +In the [readme the team has mentioned](https://github.com/sherlock-audit/2024-11-teller-finance-update/tree/main?tab=readme-ov-file#q-if-you-are-integrating-tokens-are-you-allowing-only-whitelisted-tokens-to-work-with-the-codebase-or-any-complying-with-the-standard-are-they-assumed-to-have-certain-properties-eg-be-non-reentrant-are-there-any-types-of-weird-tokens-you-want-to-integrate) that they would like to know if any wierd token breaks their contract pools + +In multiple places token amount which can become zero is transferred without checking the value is zero. This will cause these transactions to revert +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L699-L727 +```solidity + IERC20(principalToken).safeTransferFrom( + msg.sender, + address(this), + amountDue + tokensToTakeFromSender - liquidationProtocolFee + ); + + address protocolFeeRecipient = ITellerV2(address(TELLER_V2)).getProtocolFeeRecipient(); + + + IERC20(principalToken).safeTransferFrom( + msg.sender, + address(protocolFeeRecipient), + liquidationProtocolFee + ); + + + totalPrincipalTokensRepaid += amountDue; + tokenDifferenceFromLiquidations += int256(tokensToTakeFromSender - liquidationProtocolFee ); + + } else { + + uint256 tokensToGiveToSender = abs(minAmountDifference); + + + + IERC20(principalToken).safeTransferFrom( + msg.sender, + address(this), + amountDue - tokensToGiveToSender + ); +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +In case liquidation reverts (due to tokensToGiveToSender == -amountDue), the `tokenDifferenceFromLiquidations` won't be updated which will cause the value of the shares to be incorrectly high (because in reality the auction is settling at 0 price) + +### PoC + +_No response_ + +### Mitigation + +Check if amount is non-zero before transferring \ No newline at end of file diff --git a/052.md b/052.md new file mode 100644 index 0000000..31306ca --- /dev/null +++ b/052.md @@ -0,0 +1,60 @@ +Dandy Caramel Tortoise + +Medium + +# Sudden drop/increase in share value allows users who monitor to gain at the expense of the others + +### Summary + +Sudden drop/increase in share value allows users who monitor to gain at the expense of the others + +### Root Cause + +The pools value is calculated as: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L430-L440 +```solidity + function getPoolTotalEstimatedValue() + public + view + returns (uint256 poolTotalEstimatedValue_) + { + + int256 poolTotalEstimatedValueSigned = int256(totalPrincipalTokensCommitted) + + int256(totalInterestCollected) + int256(tokenDifferenceFromLiquidations) + - int256(totalPrincipalTokensWithdrawn); + + + //if the poolTotalEstimatedValue_ is less than 0, we treat it as 0. +``` +There are multiple reasons why the pools share value can sharply increase/decrease: + +1. The possible value of `tokenDifferenceFromLiquidations` for an auction ranges from (764% * amountDue , -amountDue) and is updated when the dutch auction for the collateral liquidation settles. This allows for a user to either front-run and deposit (in case the auction will be settling at a higher price) or prepare for a withdrawal and then withdraw their share at a higher price before the auction settles at a lower price inflicting the loss to the other shareholders. This also affects the new depositors since the `_minSharesAmountOut` won't be effective + +2. Interest repayment: In case of a large interest repayment, users who monitor for this can front run this repayment and then queue a withdrawal. This way they won't be deposited into the pool but will still earn the interest + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. User monitors a large repayment of interest +2. User front-runs this tx and deposits into the pool +3. User then queues withdrawal for their shares to take the profit + +### Impact + +Naive users can suffer unfair losses + +### PoC + +_No response_ + +### Mitigation + +Implementing a depsoit queue could solve the share value increase issue \ No newline at end of file diff --git a/053.md b/053.md new file mode 100644 index 0000000..a908b47 --- /dev/null +++ b/053.md @@ -0,0 +1,229 @@ +Joyful Olive Meerkat + +Medium + +# A borrower, lender, or liquidator may be unable to withdraw collateral assets if the gas limit is exceeded. + +### Summary + +Within the TellerV2#`submitBid()`, there is no limitation that how many collateral assets a borrower can assign into the `_collateralInfo` array parameter. + +This lead to some bad scenarios like this due to reaching gas limit: + +* A borrower or a lender fail to withdraw the collateral assets when the loan would not be liquidated. +* A liquidator will fail to withdraw the collateral assets when the loan would be liquidated. + + +### Root Cause + +Within the ICollateralEscrowV1, the [Collateral struct](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/interfaces/escrow/ICollateralEscrowV1.sol#L10) would be defined line this: + + +```solidity +struct Collateral { + CollateralType _collateralType; + uint256 _amount; + uint256 _tokenId; + address _collateralAddress; +} +``` + +Within the `CollateralManager`, the [CollateralInfo](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L42) struct would be defined like this: + +```solidity + /** + * Since collateralInfo is mapped (address assetAddress => Collateral) that means + * that only a single tokenId per nft per loan can be collateralized. + * Ex. Two bored apes cannot be used as collateral for a single loan. + */ + struct CollateralInfo { + EnumerableSetUpgradeable.AddressSet collateralAddresses; + mapping(address => Collateral) collateralInfo; + } +``` + +Within the CollateralManager, the [_bidCollaterals storage](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L35) would be defined like this: + +```solidity +// bidIds -> validated collateral info + mapping(uint256 => CollateralInfo) internal _bidCollaterals; +``` + +When a borrower submits a bid, the TellerV2#`submitBid()` would be called. +Within the TellerV2#`submitBid()`, multiple collaterals, which are ERC20/ERC721/ERC1155, can be assigned into the `_collateralInfo` array parameter by a borrower. +And then, these collateral assets stored into the `_collateralInfo` array would be associated with `bidId_` through internally calling the CollateralManager#`commitCollateral()` like this: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L314 + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L328 + +```solidity +function submitBid( + address _lendingToken, + uint256 _marketplaceId, + uint256 _principal, + uint32 _duration, + uint16 _APR, + string calldata _metadataURI, + address _receiver, + Collateral[] calldata _collateralInfo <@ audit + ) public override whenNotPaused returns (uint256 bidId_) { + ... + bool validation = collateralManager.commitCollateral( + bidId_, + _collateralInfo /// @audit + ); + ... +``` + +Within the [commitCollateral()](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L140), each collateral asset (`info`) would be associated with a `_bidId` respectively by calling the CollateralManager#`_commitCollateral()` in the for-loop like this: + +```solidity + /** + * @notice Checks the validity of a borrower's multiple collateral balances and commits it to a bid. + * @param _bidId The id of the associated bid. + * @param _collateralInfo Additional information about the collateral assets. + * @return validation_ Boolean indicating if the collateral balances were validated. + */ + function commitCollateral( + uint256 _bidId, + Collateral[] calldata _collateralInfo <@ audit + ) public onlyTellerV2 returns (bool validation_) { + address borrower = tellerV2.getLoanBorrower(_bidId); + require(borrower != address(0), "Loan has no borrower"); + (validation_, ) = checkBalances(borrower, _collateralInfo); + + //if the collateral info is valid, call commitCollateral for each one + if (validation_) { + for (uint256 i; i < _collateralInfo.length; i++) { + Collateral memory info = _collateralInfo[i]; + _commitCollateral(_bidId, info); <@ audit + } + } + } +``` + +Within the CollateralManager#`_commitCollateral()`, the `_collateralInfo` would be stored into the `_bidCollaterals` storage like this: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L506 + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L522-L526 + +When the deposited-collateral would be withdrawn by a borrower or a lender, the CollateralManager#`withdraw()` would be called. +Within the CollateralManager#`withdraw()`, the CollateralManager#`_withdraw()` would be called like this: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L283 + +```solidity +/** + * @notice Withdraws deposited collateral from the created escrow of a bid that has been successfully repaid. + * @param _bidId The id of the bid to withdraw collateral for. + */ + function withdraw(uint256 _bidId) external whenProtocolNotPaused { + BidState bidState = tellerV2.getBidState(_bidId); + + require(bidState == BidState.PAID, "collateral cannot be withdrawn"); + + _withdraw(_bidId, tellerV2.getLoanBorrower(_bidId)); <@ audit + + emit CollateralClaimed(_bidId); + } +``` + +When the deposited-collateral would be liquidated by a liquidator, the CollateralManager#`liquidateCollateral()` would be called. +Within the CollateralManager#`liquidateCollateral()`, the CollateralManager#`_withdraw()` would be called like this: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L355 + +```solidity + /** + * @notice Sends the deposited collateral to a liquidator of a bid. + * @notice Can only be called by the protocol. + * @param _bidId The id of the liquidated bid. + * @param _liquidatorAddress The address of the liquidator to send the collateral to. + */ + function liquidateCollateral(uint256 _bidId, address _liquidatorAddress) + external + onlyTellerV2 + whenProtocolNotPaused + { + if (isBidCollateralBacked(_bidId)) { + BidState bidState = tellerV2.getBidState(_bidId); + require( + bidState == BidState.LIQUIDATED, + "Loan has not been liquidated" + ); + _withdraw(_bidId, _liquidatorAddress); <@audit + } + } +``` + +Within the CollateralManager#`_withdraw()`, each collateral asset (`collateralInfo._collateralAddress`) would be withdrawn by internally calling the ICollateralEscrowV1#`withdraw()` in a for-loop like this: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L472-L487 + +```solidity + /** + * @notice Withdraws collateral to a given receiver's address. + * @param _bidId The id of the bid to withdraw collateral for. + * @param _receiver The address to withdraw the collateral to. + */ + function _withdraw(uint256 _bidId, address _receiver) internal virtual { + for ( + uint256 i; + i < _bidCollaterals[_bidId].collateralAddresses.length(); <@audit + i++ + ) { + // Get collateral info + Collateral storage collateralInfo = _bidCollaterals[_bidId] + .collateralInfo[ + _bidCollaterals[_bidId].collateralAddresses.at(i) + ]; + // Withdraw collateral from escrow and send it to bid lender + ICollateralEscrowV1(_escrows[_bidId]).withdraw( <@ audit + collateralInfo._collateralAddress, + collateralInfo._amount, + _receiver + ); + .... + } + } +``` + +However, within the TellerV2#`submitBid()`, there is no limitation that how many collateral assets a borrower can assign into the `_collateralInfo` array parameter. + +This lead to a bad scenario like below: + +* ① A borrower assign too many number of the collateral assets (ERC20/ERC721/ERC1155) into the `_collateralInfo` array parameter when the borrower call the TellerV2#`submitBid()` to submit a bid. +* ② Then, a lender accepts the bid via calling the TellerV2#`lenderAcceptBid()` +* ③ Then, a borrower or a lender try to withdraw the collateral, which is not liquidated, by calling the CollateralManager#`withdraw()`. Or, a liquidator try to withdraw the collateral, which is liquidated, by calling the CollateralManager#`liquidateCollateral()` +* ④ But, the transaction of the CollateralManager#`withdraw()` or the CollateralManager#`liquidateCollateral()` will be reverted in the for-loop of the CollateralManager#`_withdraw()` because that transaction will reach a gas limit. + + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Due to reaching gas limit, some bad scenarios would occur like this: + +* A borrower or a lender fail to withdraw the collateral assets when the loan would not be liquidated. +* A liquidator will fail to withdraw the collateral assets when the loan would be liquidated. + + +### PoC + +_No response_ + +### Mitigation + +Within the TellerV2#`submitBid()`, consider adding a limitation about how many collateral assets a borrower can assign into the `_collateralInfo` array parameter. diff --git a/054.md b/054.md new file mode 100644 index 0000000..a47d863 --- /dev/null +++ b/054.md @@ -0,0 +1,71 @@ +Bubbly Inky Sawfish + +Medium + +# The `totalPrincipalTokensRepaid` and `totalInterestCollected` may not be updated even when funds are already transferred + +### Summary +The `LenderCommitmentGroup_Smart.repayLoanCallback()` function will be paused, causing the transaction to continue despite the revert. As a result, while the funds are transferred, the amounts will not be added to `totalPrincipalTokensRepaid` and `totalInterestCollected`. This discrepancy will lead to an incorrect calculation of the exchange rate, potentially resulting in a loss of funds for shareholders. + +### Root Cause + +The `LenderCommitmentGroup_Smart.repayLoanCallback()` function will revert due to being paused. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-L945 +```solidity + function repayLoanCallback( + uint256 _bidId, + address repayer, + uint256 principalAmount, + uint256 interestAmount +@> ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { + totalPrincipalTokensRepaid += principalAmount; + totalInterestCollected += interestAmount; + + emit LoanRepaid( + _bidId, + repayer, + principalAmount, + interestAmount, + totalPrincipalTokensRepaid, + totalInterestCollected + ); + } +``` + +However, the whole transaction will not be reverted because of the try/catch statement. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L938-950 +```solidity + if (loanRepaymentListener != address(0)) { + require(gasleft() >= 80000, "NR gas"); //fixes the 63/64 remaining issue +@> try + ILoanRepaymentListener(loanRepaymentListener).repayLoanCallback{ + gas: 80000 + }( //limit gas costs to prevent lender preventing repayments + _bidId, + _msgSenderForMarket(bid.marketplaceId), + _payment.principal, + _payment.interest + ) +@> {} catch {} + } +``` + +Borrowers can repay their loans even during a pause. This means that while the funds are transferred, the amounts will not be added to `totalPrincipalTokensRepaid` and `totalInterestCollected`. Consequently, the exchange rate will be calculated incorrectly, which could result in a loss of funds for shareholders. + +### Internal pre-conditions +none + +### External pre-conditions +none + +### Attack Path +none + +### Impact +Loss of fund to shareholders. + +### PoC +none + +### Mitigation +The `LenderCommitmentGroup_Smart.repayLoanCallback()` function should not revert when paused. \ No newline at end of file diff --git a/055.md b/055.md new file mode 100644 index 0000000..a248eb8 --- /dev/null +++ b/055.md @@ -0,0 +1,51 @@ +Petite Pewter Orangutan + +High + +# No access control on `setCollateralEscrowBeacon` + +### Summary + +[CollateralManager::setCollateralEscrowBeacon](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L114) can be called by anyone. + +### Root Cause + +_No response_ + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +An attacker can set the escrow beacon and drain funds. +As we can see it has `reinitializer`, which means it can be changed. + +### PoC + +[CollateralManager::setCollateralEscrowBeacon](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L114) + +```solidity + /** + * @notice Sets the address of the Beacon contract used for the collateral escrow contracts. + * @param _collateralEscrowBeacon The address of the Beacon contract. + */ + function setCollateralEscrowBeacon(address _collateralEscrowBeacon) + external + reinitializer(2) + { + collateralEscrowBeacon = _collateralEscrowBeacon; + } +``` + +### Mitigation + +for `setCollateralEscrowBeacon()` set `onlyTellerV2` modifier. \ No newline at end of file diff --git a/056.md b/056.md new file mode 100644 index 0000000..36b56d0 --- /dev/null +++ b/056.md @@ -0,0 +1,73 @@ +Clever Mocha Pheasant + +Medium + +# Missing Escrow Address Storage in Proxy Deployment Function + +## Summary + +The CollateralManager contract contains a critical vulnerability in its `_deployEscrow` function where it fails to store newly deployed escrow proxy addresses in the contract's state. The function correctly creates new BeaconProxy instances but only assigns the address to a return variable without updating the `_escrows` mapping. + +This vulnerability fundamentally breaks the collateral management system as it prevents the contract from tracking and accessing deployed escrow contracts. The impact is severe because: + +1. Multiple unused escrow contracts could be deployed for the same bid ID since each call with `_escrows[_bidId] == 0` creates a new proxy +2. Any subsequent attempts to interact with the escrow through functions like `_deposit` and `_withdraw` will fail or interact with the wrong contract since they rely on `_escrows[_bidId]` to find the correct escrow +3. This could lead to permanent loss of user collateral as assets could be sent to escrow contracts that become inaccessible +4. The contract wastes significant gas through repeated unnecessary proxy deployments + +The vulnerability stems from the following code section: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L365 + +```solidity +function _deployEscrow(uint256 _bidId) + internal + virtual + returns (address proxyAddress_, address borrower_) +{ + proxyAddress_ = _escrows[_bidId]; + borrower_ = tellerV2.getLoanBorrower(_bidId); + if (proxyAddress_ == address(0)) { + require(borrower_ != address(0), "Bid does not exist"); + + BeaconProxy proxy = new BeaconProxy( + collateralEscrowBeacon, + abi.encodeWithSelector( + ICollateralEscrowV1.initialize.selector, + _bidId + ) + ); + proxyAddress_ = address(proxy); + // Missing: _escrows[_bidId] = proxyAddress_; + } +} +``` + +## Recommended mitigation steps +The fix requires updating the `_escrows` mapping whenever a new proxy is deployed. Here's the corrected implementation: + +```solidity +function _deployEscrow(uint256 _bidId) + internal + virtual + returns (address proxyAddress_, address borrower_) +{ + proxyAddress_ = _escrows[_bidId]; + borrower_ = tellerV2.getLoanBorrower(_bidId); + if (proxyAddress_ == address(0)) { + require(borrower_ != address(0), "Bid does not exist"); + + BeaconProxy proxy = new BeaconProxy( + collateralEscrowBeacon, + abi.encodeWithSelector( + ICollateralEscrowV1.initialize.selector, + _bidId + ) + ); + proxyAddress_ = address(proxy); + _escrows[_bidId] = proxyAddress_; // Store the escrow address + } +} +``` + +These changes ensure proper tracking of deployed escrow contracts and maintain the integrity of the collateral management system while preventing potential loss of user funds. \ No newline at end of file diff --git a/057.md b/057.md new file mode 100644 index 0000000..cbe9aac --- /dev/null +++ b/057.md @@ -0,0 +1,75 @@ +Clever Mocha Pheasant + +Medium + +# Incomplete Validation State in Collateral Balance Checks + +## Summary + +The CollateralManager contract contains a significant vulnerability in its collateral validation mechanism where the validation state becomes unreliable due to premature returns during short-circuit operations. The issue manifests in the `_checkBalances` function which attempts to optimize gas usage by allowing early termination of validation checks, but in doing so creates undefined states that could compromise the integrity of the collateral management system. + +When performing balance validations with `_shortCircuit` enabled, the function returns upon encountering the first failed check without properly initializing the remaining validation states. This breaks the contract's ability to provide accurate validation information to dependent systems and could lead to incorrect collateral management decisions. + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L541 + +```solidity +function _checkBalances( + address _borrowerAddress, + Collateral[] memory _collateralInfo, + bool _shortCircuit +) internal virtual view returns (bool validated_, bool[] memory checks_) { + checks_ = new bool[](_collateralInfo.length); + validated_ = true; + for (uint256 i; i < _collateralInfo.length; i++) { + bool isValidated = _checkBalance( + _borrowerAddress, + _collateralInfo[i] + ); + checks_[i] = isValidated; + if (!isValidated) { + validated_ = false; + //if short circuit is true, return on the first invalid balance to save execution cycles. + //Values of checks[] will be invalid/undetermined if shortcircuit is true. + if (_shortCircuit) { + return (validated_, checks_); + } + } + } +} +``` + +The implications of this vulnerability extend beyond simple data integrity. Contracts or interfaces relying on the validation results may misinterpret undefined states as successful validations, potentially allowing invalid collateral to be processed. This creates a systemic risk where the optimization for gas efficiency compromises the core security guarantees of the collateral validation system. + +## Recommended mitigation steps +To maintain both gas efficiency and data integrity, the validation mechanism should be modified to ensure complete and accurate state information even during short-circuit operations. The following implementation addresses these concerns while preserving the gas optimization benefits: + +```solidity +function _checkBalances( + address _borrowerAddress, + Collateral[] memory _collateralInfo, + bool _shortCircuit +) internal virtual view returns (bool validated_, bool[] memory checks_) { + checks_ = new bool[](_collateralInfo.length); + validated_ = true; + + for (uint256 i; i < _collateralInfo.length; i++) { + bool isValidated = _checkBalance( + _borrowerAddress, + _collateralInfo[i] + ); + checks_[i] = isValidated; + + if (!isValidated) { + validated_ = false; + if (_shortCircuit) { + for (uint256 j = i + 1; j < _collateralInfo.length; j++) { + checks_[j] = false; + } + return (validated_, checks_); + } + } + } +} +``` + +This solution implements a deterministic approach to validation state management. When short-circuiting occurs, the function explicitly marks remaining validations as failed rather than leaving them in an undefined state. This maintains the gas efficiency of early termination while ensuring that all consumers of the validation data receive complete and accurate information about the validation state. diff --git a/058.md b/058.md new file mode 100644 index 0000000..1395614 --- /dev/null +++ b/058.md @@ -0,0 +1,58 @@ +Clever Mocha Pheasant + +Medium + +# Incomplete Token Permission Validation in Collateral Checks + +## Summary +The CollateralManager contract contains a vulnerability in its collateral validation logic where it fails to verify token spending permissions. The `_checkBalance` function only validates token balances while ignoring crucial allowance checks, which can lead to failed transactions and inconsistent contract state. + +The vulnerability manifests in the ERC20 validation logic where the function performs an incomplete verification: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L576 + +```solidity +if (collateralType == CollateralType.ERC20) { + return + _collateralInfo._amount <= + IERC20(_collateralInfo._collateralAddress).balanceOf( + _borrowerAddress + ); +} +``` + +This implementation naively assumes that having sufficient balance is the only requirement for collateral deposit. However, in ERC20's design, both balance and allowance are required for token transfers. The impact is significant as it creates a false sense of security where collateral validation passes but the actual deposit operation fails due to insufficient allowance. + +This design flaw ripples through the system because a successful balance check leads users and integrating protocols to believe the collateral can be deposited. However, the subsequent `depositAndDeploy` operation will revert when attempting to execute `safeTransferFrom`, resulting in failed transactions and wasted gas. + +## Recommended mitigation steps +The validation logic should be enhanced to verify both balance and allowance for ERC20 tokens, ensuring that all requirements for successful collateral deposit are met: + +```solidity +if (collateralType == CollateralType.ERC20) { + IERC20 token = IERC20(_collateralInfo._collateralAddress); + uint256 amount = _collateralInfo._amount; + + // Check both balance and allowance + return amount <= token.balanceOf(_borrowerAddress) && + amount <= token.allowance(_borrowerAddress, address(this)); +} +``` + +This solution provides comprehensive validation by ensuring that: +1. The borrower has sufficient token balance +2. The CollateralManager has permission to transfer the required amount +3. The deposit operation will have all necessary conditions met + +Similar validation could be extended to ERC721 and ERC1155 tokens, though these are less critical as approvals can be handled separately: + +```solidity +if (collateralType == CollateralType.ERC721) { + IERC721Upgradeable nft = IERC721Upgradeable(_collateralInfo._collateralAddress); + return _borrowerAddress == nft.ownerOf(_collateralInfo._tokenId) && + (nft.isApprovedForAll(_borrowerAddress, address(this)) || + nft.getApproved(_collateralInfo._tokenId) == address(this)); +} +``` + +Through these changes, the contract provides reliable validation that accurately reflects all requirements for successful collateral deposit, improving user experience and system reliability while preventing unnecessary gas consumption from failed transactions. \ No newline at end of file diff --git a/059.md b/059.md new file mode 100644 index 0000000..ab46f31 --- /dev/null +++ b/059.md @@ -0,0 +1,101 @@ +Clever Mocha Pheasant + +Medium + +# Share Preparation Time Lock Bypass Through State Overwrite + +## Summary + +The _prepareSharesForBurn function's implementation creates a race condition where existing share preparations can be maliciously overwritten, effectively bypassing the mandatory withdrawal delay period intended to protect against flash loan attacks and market manipulation. + +The issue lies in the function's naive state management approach where it directly overwrites the poolSharesPreparedToWithdrawForLender mapping without validating the temporal constraints of existing preparations. This implementation flaw allows an attacker to reset their withdrawal timer at will by submitting new preparations, circumventing the core security mechanism of the protocol's withdrawal system. + +This vulnerability undermines the fundamental security assumptions of the withdrawal delay mechanism. An attacker could exploit this to perform rapid withdrawals that should be time-locked, potentially leading to market manipulation through flash loan attacks or front-running opportunities. + +The vulnerability exists in the following implementation: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L99 + +```solidity +function _prepareSharesForBurn( + address _recipient, + uint256 _amountPoolSharesTokens +) internal returns (bool) { + require(balanceOf(_recipient) >= _amountPoolSharesTokens); + poolSharesPreparedToWithdrawForLender[_recipient] = _amountPoolSharesTokens; + poolSharesPreparedTimestamp[_recipient] = block.timestamp; + emit SharesPrepared( + _recipient, + _amountPoolSharesTokens, + block.timestamp + ); + return true; +} +``` + +To demonstrate the exploit, consider the following attack scenario: + +```solidity +// Initial state: Attacker has 1000 shares +// Time: T0 +await lenderShares.prepareSharesForBurn(attacker, 1000); + +// Time: T0 + 12 hours (half of delay period) +// Withdrawal should still be locked for 12 more hours +// However, attacker can reset timer: +await lenderShares.prepareSharesForBurn(attacker, 1); + +// Previous 1000 share preparation is wiped out +// New 24-hour period starts for just 1 share +// Original shares are now free from time lock +``` + +Through this mechanism, an attacker can continuously reset their withdrawal timer while maintaining control over their shares, effectively nullifying the protocol's time-lock protection mechanism. + +## Recommended mitigation steps +The vulnerability should be addressed by implementing proper state management and temporal validation in the preparation process. The following implementation provides a comprehensive fix: + +```solidity +error PreparationStillPending(uint256 timeRemaining); + +function _prepareSharesForBurn( + address _recipient, + uint256 _amountPoolSharesTokens +) internal returns (bool) { + uint256 currentBalance = balanceOf(_recipient); + require(currentBalance >= _amountPoolSharesTokens, "Insufficient balance"); + + uint256 existingPrepared = poolSharesPreparedToWithdrawForLender[_recipient]; + if (existingPrepared > 0) { + uint256 timeElapsed = block.timestamp - poolSharesPreparedTimestamp[_recipient]; + if (timeElapsed < withdrawDelayTimeSeconds) { + revert PreparationStillPending(withdrawDelayTimeSeconds - timeElapsed); + } + + // Reset expired preparation before accepting new one + poolSharesPreparedToWithdrawForLender[_recipient] = 0; + poolSharesPreparedTimestamp[_recipient] = 0; + } + + poolSharesPreparedToWithdrawForLender[_recipient] = _amountPoolSharesTokens; + poolSharesPreparedTimestamp[_recipient] = block.timestamp; + + emit SharesPrepared(_recipient, _amountPoolSharesTokens, block.timestamp); + return true; +} +``` + +This solution enforces strict temporal validation of preparation states and provides clear error reporting through custom errors. Additionally, it's recommended to implement a cancellation mechanism to allow users to explicitly void their preparations if needed, rather than relying on overwrites: + +```solidity +function cancelSharesPreparation(address _recipient) external onlyOwner { + require(poolSharesPreparedToWithdrawForLender[_recipient] > 0, "No active preparation"); + + poolSharesPreparedToWithdrawForLender[_recipient] = 0; + poolSharesPreparedTimestamp[_recipient] = 0; + + emit SharesPreparationCancelled(_recipient); +} +``` + +These changes ensure the withdrawal delay mechanism maintains its integrity while providing better user control and visibility into the preparation process. \ No newline at end of file diff --git a/060.md b/060.md new file mode 100644 index 0000000..ae76b1a --- /dev/null +++ b/060.md @@ -0,0 +1,81 @@ +Dandy Caramel Tortoise + +Medium + +# marketForwarder for a specific market can spoof a user all markets + +### Summary + +Marketforwarder is shared across all markets for a user instead of just the intended market + +### Root Cause + +The `approveMarketForwarder` is supposed to approve a forwarder contract [for a specific market](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2Context.sol#L87-L100). + +```solidity + * @notice Approves a forwarder contract to use their address as a sender for a specific market. + * @notice The forwarder given must be trusted by the market given. + * @param _marketId An ID for a lending market. + * @param _forwarder A forwarder contract address. + */ + function approveMarketForwarder(uint256 _marketId, address _forwarder) + external + { + require( + isTrustedMarketForwarder(_marketId, _forwarder), + "Forwarder must be trusted by the market" + ); + _approvedForwarderSenders[_forwarder].add(_msgSender()); + emit MarketForwarderApproved(_marketId, _forwarder, _msgSender()); +``` + +But this is not followed. If a user approves a forwarder for one market, that [forwarder can spoof the user for any other market in which they are considered trusted](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2Context.sol#L122-L140) + +```solidity + function _msgSenderForMarket(uint256 _marketId) + internal + view + virtual + returns (address) + { + if ( + msg.data.length >= 20 && + isTrustedMarketForwarder(_marketId, _msgSender()) + ) { + address sender; + assembly { + sender := shr(96, calldataload(sub(calldatasize(), 20))) + } + // Ensure the appended sender address approved the forwarder + // @audit no market wise seperation + require( +=> _approvedForwarderSenders[_msgSender()].contains(sender), + "Sender must approve market forwarder" + ); +``` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +1. User approves A as a trusted forwarder for MA +2. A is also made the trusted forwarder of MB and MC by the respective owners +3. A can now spoof the user for MB and MC also which they have not intended + +### Impact + +The trusted forwarder for a market can spoof the user across all markets. This can result in bids and lendings for terms that the user doesn't align with + +### PoC + +_No response_ + +### Mitigation + +Seperate forwarders marketwise \ No newline at end of file diff --git a/061.md b/061.md new file mode 100644 index 0000000..9624967 --- /dev/null +++ b/061.md @@ -0,0 +1,90 @@ +Clever Mocha Pheasant + +Medium + +# Unprotected Price Oracle Reading Enables Single-Block Price Manipulation + +## Summary + +The UniswapPricingLibrary contains a critical vulnerability in its price fetching mechanism where it allows direct access to the current pool price through slot0 when twapInterval is set to zero. The core issue lies in the getSqrtTwapX96 function which bypasses Uniswap V3's time-weighted average price (TWAP) protection when twapInterval equals zero, making it susceptible to flash loan attacks and price manipulation within a single block. + +This vulnerability stems from the assumption that instantaneous price readings from slot0 are acceptable alternatives to TWAP. However, in the context of Uniswap V3's architecture, slot0 prices can be manipulated through large swaps within a single transaction, especially using flash loans. The impact is severe because any protocol relying on this library for price feeds could be exploited through price manipulation, potentially leading to significant financial losses through oracle attacks on lending, derivatives, or other DeFi protocols. + +The vulnerability manifests in the following code path: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/UniswapPricingLibrary.sol#L105 + +```solidity +function getSqrtTwapX96(address uniswapV3Pool, uint32 twapInterval) + internal + view + returns (uint160 sqrtPriceX96) +{ + if (twapInterval == 0) { + (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(uniswapV3Pool).slot0(); + } + // ... +} +``` + +An attacker can exploit this by executing the following attack flow in a single transaction: +1. Obtain a flash loan +2. Execute a large swap to manipulate the pool's current price +3. Take advantage of protocols using the vulnerable price reading +4. Profit from the manipulation +5. Repay the flash loan + +## Recommended mitigation steps +The vulnerability should be addressed through a comprehensive overhaul of the price reading mechanism. Instead of allowing direct slot0 access, the library should implement a robust oracle system that ensures price reliability. + +The fix involves modifying the getSqrtTwapX96 function to enforce TWAP usage in all scenarios. This can be achieved by removing the twapInterval=0 case entirely and implementing a minimum TWAP interval requirement: + +```solidity +function getSqrtTwapX96(address uniswapV3Pool, uint32 twapInterval) + internal + view + returns (uint160 sqrtPriceX96) +{ + // Enforce minimum TWAP interval + uint32 effectiveInterval = Math.max(twapInterval, MIN_TWAP_PERIOD); + + uint32[] memory secondsAgos = new uint32[](2); + secondsAgos[0] = effectiveInterval + 1; + secondsAgos[1] = 1; + + (int56[] memory tickCumulatives, ) = IUniswapV3Pool(uniswapV3Pool) + .observe(secondsAgos); + + // Calculate TWAP price + sqrtPriceX96 = TickMath.getSqrtRatioAtTick( + int24((tickCumulatives[1] - tickCumulatives[0]) / + int32(effectiveInterval)) + ); +} +``` + +For cases requiring more recent price data, implement a separate function that combines TWAP with additional safety checks: + +```solidity +function getRecentPrice(address uniswapV3Pool) + internal + view + returns (uint160 sqrtPriceX96) +{ + // Get current price + (uint160 currentPrice, , , , , , ) = IUniswapV3Pool(uniswapV3Pool).slot0(); + + // Get short TWAP for comparison + uint160 twapPrice = getSqrtTwapX96(uniswapV3Pool, SHORT_TWAP_PERIOD); + + // Ensure current price hasn't deviated significantly from TWAP + require( + calculateDeviation(currentPrice, twapPrice) <= MAX_DEVIATION, + "Price deviation too high" + ); + + return currentPrice; +} +``` + +This solution provides strong protection against price manipulation while maintaining the ability to access recent price data when needed. The implementation should be accompanied by thorough testing across various market conditions and proper documentation of the security assumptions and limitations. \ No newline at end of file diff --git a/062.md b/062.md new file mode 100644 index 0000000..3739f41 --- /dev/null +++ b/062.md @@ -0,0 +1,88 @@ +Dandy Caramel Tortoise + +Medium + +# Incorrect messge sender in case base `_trustedForwarder` is used + +### Summary + +The last 20 bytes could be used to identify both the market forwarder and also the market user in case the market forwarder uses the base forwarder themselves + +### Root Cause + +`_msgSenderForMarket` decodes the last 20 bytes as message sender in case the current sender is a trusted forwarder for the market + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2Context.sol#L122-L140 +```solidity + function _msgSenderForMarket(uint256 _marketId) + internal + view + virtual + returns (address) + { + if ( + msg.data.length >= 20 && + isTrustedMarketForwarder(_marketId, _msgSender()) + ) { + address sender; + assembly { + sender := shr(96, calldataload(sub(calldatasize(), 20))) + } + // Ensure the appended sender address approved the forwarder + require( + _approvedForwarderSenders[_msgSender()].contains(sender), + "Sender must approve market forwarder" + ); + return sender; + } + + return _msgSender(); + } +``` + +But in case the trusted forwarder themselves is using the base trusted forwarder ie. `_trustedForwarder`, the last 20 bytes will be the address of the market's trusted forwarder +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/ERC2771ContextUpgradeable.sol#L34-L50 +```solidity + function _msgSender() + internal + view + virtual + override + returns (address sender) + { + if (isTrustedForwarder(msg.sender)) { + // The assembly code is more direct than the Solidity version using `abi.decode`. + assembly { + sender := shr(96, calldataload(sub(calldatasize(), 20))) + } + } else { + return super._msgSender(); + } + } +``` + +Hence in such a scenario the decoding will be incorrect + +### Internal pre-conditions + +A market's trusted forwarder should use the base trusted forwarder themselves + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Incorrect decoding of the message sender causing fund transfer etc. to happen from the incorrect account + +### PoC + +_No response_ + +### Mitigation + +Avoid using base trusted forwarder by market trusted providers \ No newline at end of file diff --git a/063.md b/063.md new file mode 100644 index 0000000..785b4be --- /dev/null +++ b/063.md @@ -0,0 +1,195 @@ +Wild Chili Nuthatch + +High + +# Implementing Front-Running Protection in Smart Contracts: Commit-Reveal and Time-Based Lock Mechanisms + +***Summary*** +[https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-L945](url) + +The code lacks front-running protection mechanisms, which makes it vulnerable to attacks where malicious actors can anticipate and exploit transactions. Specifically, the absence of commit-reveal or time-based locks allows for the possibility of front-running in functions such as loan liquidation or bidding. This could lead to unintended outcomes like unfair price manipulation or attackers profiting from knowing bid details in advance. + +***Root Cause*** +The root cause of the front-running vulnerability is the absence of any protection mechanism that would obscure the details of sensitive transactions (e.g., loan liquidations or bids) before they are finalized. Without commit-reveal schemes or time-based locks, the transaction details (such as bid amounts or liquidation parameters) are visible to all participants in the network, including potential attackers. This allows them to take advantage of this information by submitting their own transactions ahead of the legitimate ones, altering the outcome in their favor. + + +***Internal pre-conditions*** + +Lack of Transaction Privacy: + +Transaction details such as bid amounts, liquidation conditions, or loan parameters are exposed on the blockchain before they are processed. This allows any participant to view the data, including potential front-runners. +No Commit-Reveal Mechanism: + +There is no commit-reveal mechanism, which would normally allow participants to first commit to a transaction with a hash and later reveal the actual details. Without this mechanism, the details of bids or transactions are immediately available, enabling malicious actors to exploit the timing of their transactions. +Absence of Time-Based Locks: + +No time-based lock or delay is implemented to prevent transactions from being executed too quickly or prematurely. This means that attackers can observe transaction data (such as price feeds or bid details) and submit their own transactions before the original transaction is mined. +These conditions allow the critical vulnerability of front-running, where attackers can take advantage of observable pending transactions to change the market conditions in their favor. + +***External pre-conditions*** + +The external pre-conditions leading to the front-running vulnerability are: + +Unpredictable Transaction Ordering: + +Since Ethereum and other blockchain networks typically do not guarantee the order in which transactions are included in a block, miners or validators can choose to prioritize transactions based on gas price or other factors. This lack of transaction ordering guarantees can be exploited by front-runners who can submit their own transactions with higher gas fees to ensure their transactions are processed first. +Publicly Available Transaction Data: + +Transactions in the mempool (the pool of pending transactions) are publicly visible to all participants. Front-runners can monitor the mempool for specific types of transactions (such as large bids, liquidations, or loan actions) and then attempt to submit their own transactions based on this visible data. +Unprotected External Calls (e.g., Oracle Prices or Auctions): + +If external data sources, such as oracles (for price data) or auctions, are used in the logic of the smart contract, they may expose information about pending transactions or market conditions. If such data is accessible before the transaction is finalized, it can allow attackers to predict the outcome of a transaction and front-run it. +Lack of Front-End or Off-Chain Safeguards: + +If the system’s front-end (user interface) or off-chain processes expose sensitive information (such as bids, collateral, or liquidation conditions) before the on-chain transaction is executed, it can provide an opportunity for external participants to exploit this information for front-running. + +***Attack Path*** + +1. Observation of Pending Transactions: +Step 1: Malicious actors (front-runners) continuously monitor the mempool for transactions that involve significant amounts of money or opportunities for profit, such as loan liquidations, large bids, or contract function calls like repayLoanCallback, liquidateLoan, or borrowLoan. +Step 2: The front-runner identifies a transaction that could affect the price of collateral or principal tokens, such as a loan repayment or liquidation. +2. Analysis of Transaction Conditions: +Step 3: The front-runner analyzes the details of the transaction in the mempool (publicly visible information such as loan amounts, collateral, time window, etc.) to understand how the transaction will affect the state of the protocol. This could include, for example: +Understanding the potential price fluctuation due to collateral token movements in an auction. +Recognizing a liquidateLoan transaction where the collateral is being sold at an unfavorable price for the borrower. +3. Reconstruction of Transaction Outcome: +Step 4: The front-runner then reconstructs the expected outcome of the transaction: +If it's a liquidation, they could estimate the collateral-to-principal token ratio or the incentive to liquidate (based on price fluctuations or auction behavior). +If it's a loan repayment, they could estimate the changes in available liquidity or interest rates based on the transaction. +4. Execution of Front-Running Transaction: +Step 5: The front-runner submits their own transaction with higher gas fees to ensure their transaction is processed before the original transaction. +Step 6: The front-runner’s transaction is executed first, allowing them to either: +Manipulate the price of collateral by engaging in trades or auction bids, making the liquidation more favorable for them. +Disrupt the liquidation auction by participating in the liquidation process before the original transaction. +Extract profits by exploiting the price discrepancy created by the transaction. +5. Completion of Malicious Transaction: +Step 7: The front-runner's transaction is successfully processed before the original transaction, and they capitalize on the changes they made to the protocol state. +Step 8: The originally intended transaction (e.g., loan repayment or liquidation) executes with its expected outcome, but now the attacker has gained an advantage, such as better collateral prices or a more favorable liquidation. +6. Profit Extraction: +Step 9: The front-runner can extract profit from the manipulated transaction by either: +Taking advantage of arbitrage opportunities created by price manipulation in collateral or principal tokens. +Benefiting from liquidations where they can acquire tokens at a discounted price due to their prior intervention. +7. Repeat: +Step 10: The attacker continues to observe transactions in the mempool and repeat the process, exploiting future transactions in a similar manner. +Key Exploitable Vulnerabilities in the Path: +Public Transaction Data: The ability to observe pending transactions in the mempool, such as loan liquidations or significant transfers, allows the front-runner to predict the effects of these transactions. +Mempool Visibility: Transactions like repayLoanCallback, liquidateLoan, or borrowLoan can be used as indicators for front-running, especially if the conditions or amounts are sufficiently large or impactful. +No Mechanism to Prevent Reordering: The lack of a commit-reveal mechanism or other ordering protections allows malicious actors to exploit transaction reordering by miners/validators for profit. + +***Impact*** + +1. Financial Losses to Users +Lenders and Borrowers: Front-running can result in users paying more for collateral or receiving less favorable terms during loan repayments or liquidations. For example: +Lenders could experience reduced returns on their investment if a front-runner manipulates the collateral prices, resulting in a less profitable liquidation process or market conditions. +Borrowers might face higher costs for liquidations if a malicious actor front-runs a liquidation transaction, acquiring the collateral at a price that they would have otherwise paid. +Profit Reduction: If an attacker manipulates prices, users may end up paying more than they should for liquidations or token exchanges due to artificially inflated prices. Similarly, when prices are deflated, the attacker could benefit at the expense of the protocol or other users. +2. Loss of Protocol Integrity and Trust +Market Manipulation: Repeated front-running can lead to market manipulation, undermining the integrity of the protocol. Users may begin to distrust the system, especially if they perceive that attackers can exploit transactions for personal gain. +Reputation Damage: If front-running becomes known within the community or broader market, it could seriously damage the protocol’s reputation. Users expect fair treatment in financial markets, and manipulation undermines confidence in decentralized finance (DeFi) protocols. +Reduced Participation: Users may become less willing to participate in the ecosystem if they fear they will be manipulated by front-runners, leading to lower transaction volumes and liquidity in the protocol. +3. Increased Gas Costs +Higher Transaction Fees: Front-runners typically outbid the original transaction in terms of gas price to ensure that their transactions are processed first. This can artificially inflate gas prices, making transactions more expensive for everyone, especially if the front-runner's strategy requires multiple attempts or increased competition with other attackers. +Inefficient Capital Usage: The increased gas costs and the need for frequent re-submissions of transactions can lead to inefficient use of capital for the protocol's users, raising the barrier to entry for smaller participants. +4. Impact on Liquidations +Unfair Liquidation Auctions: In liquidation events, front-running can skew the price at which collateral is liquidated. If an attacker is able to front-run a liquidation, they may acquire the collateral at a significantly reduced price, harming the protocol's liquidation process. This could leave the protocol with insufficient collateral backing, especially in cases where the liquidation amount is critical for ensuring solvency. +Financial Inefficiency: Front-running can prevent optimal liquidation outcomes, which might result in liquidations that are less profitable or fail to adequately cover the borrowed funds. This could leave lenders with a portion of their capital unrecovered. +5. Impact on Incentive Mechanisms +Skewed Incentives: Many incentive mechanisms in DeFi protocols depend on the price of tokens, the amount of collateral, or the amount of liquidity provided. Front-running can distort these incentives, making it harder for users to accurately predict their rewards or the value of their investments. +Reduced Effectiveness of Incentives: If an attacker can manipulate liquidation incentives or price adjustments, the protocol’s incentive structure may no longer effectively encourage the desired behaviors (e.g., liquidity provision or loan repayment). +6. Regulatory and Compliance Risks +Regulatory Attention: The occurrence of front-running attacks might draw unwanted regulatory attention. Authorities may start to scrutinize the protocol more closely, considering whether these attacks constitute unfair market practices or breach laws related to market manipulation. +Legal Exposure: If the protocol is found to be vulnerable to front-running, it could face legal challenges from users who believe they were unfairly impacted by such attacks, leading to potential lawsuits or reputational damage. + +***PoC*** + +Assumptions: +The protocol relies on price queries from Uniswap pools (getUniswapV3TokenPairPrice), which can be manipulated by timing the execution of transactions. +No protection against front-running is implemented, so an attacker can place their transaction before the actual liquidation, which results in them acquiring the collateral at a favorable price. +PoC Attack Steps +Step 1: Set up a Liquidation Transaction + +A borrower has defaulted on their loan, triggering a liquidation event. +A liquidation transaction is created, which will attempt to exchange collateral for the principal tokens, but the transaction has not yet been executed on-chain. +Step 2: Attacker Monitors Pending Transactions + +The attacker observes the pending liquidation transaction, identifying the getUniswapV3TokenPairPrice or price oracle functions that will be used to execute the liquidation. +The attacker identifies the price at which collateral will be exchanged (via getUniswapV3TokenPairPrice). +Step 3: Attacker Front-Runs the Transaction + +The attacker creates their own transaction, setting a higher gas price to ensure that it is included before the original liquidation transaction. +The attacker's transaction manipulates the price slightly in their favor by performing a small trade or interaction with the Uniswap V3 pool. +Since the liquidation depends on the price returned by the getUniswapV3TokenPairPrice function, which is based on the current state of the Uniswap pool, the attacker is able to influence the collateral-to-principal exchange rate by modifying the pool price. +Step 4: Liquidation Executed at a Different Price + +The liquidation transaction is now processed after the attacker's transaction, and the collateral is exchanged at an unfavorable price for the borrower (the price has been adjusted in the attacker's favor). +The attacker profits from acquiring the collateral at a more favorable rate than the borrower would have received. +Step 5: The Attacker Reaps the Profits + +The attacker receives the collateral tokens at the manipulated price, thereby acquiring the collateral at a value lower than what it should have been, leading to a financial gain. +Example Code for Simulating the Attack + +```solidity + +// Example of an attacker contract simulating front-running behavior + +pragma solidity ^0.8.0; + +interface IUniswapV3Pool { + function getSqrtTwapX96(uint32 twapInterval) external view returns (uint160); + function observe(uint32[] calldata secondsAgos) external view returns (int56[] memory, uint160[] memory); +} + +contract Attacker { + IUniswapV3Pool uniswapPool; + + constructor(address _uniswapPool) { + uniswapPool = IUniswapV3Pool(_uniswapPool); + } + + // Function to front-run a liquidation event + function frontRunLiquidation(uint256 amountToManipulate) external { + // Step 1: Check the current price from Uniswap pool + uint160 currentSqrtPrice = uniswapPool.getSqrtTwapX96(0); // Current price without any manipulation + + // Step 2: Manipulate the price by swapping tokens in the pool + // This causes the price to shift, impacting the liquidation price + // A small swap that alters the price + manipulatePrice(amountToManipulate); + + // Step 3: Observe the new price after manipulation + uint160 manipulatedPrice = uniswapPool.getSqrtTwapX96(0); + + // Step 4: Execute the liquidation with the manipulated price + // Assuming the liquidation contract calls _getUniswapV3TokenPairPrice internally + // The attacker can now exploit the difference in prices + + // Example interaction with liquidation function (simplified) + // liquidationContract.liquidate(amountToManipulate, manipulatedPrice); + } + + // Function to manipulate the price of the token pair + function manipulatePrice(uint256 amount) internal { + // Swap a small amount of tokens to change the pool price slightly + // The attacker could use any logic to manipulate the price + // For simplicity, we just execute a small trade (this depends on the token interaction) + // In a real case, the attacker would perform trades that affect the price oracle + + // Example of a "manipulation" logic (simplified) + uint256 manipulatedAmount = amount; // Adjust the amount as needed + // Example: perform a swap in the Uniswap V3 pool to shift the price + // uniswapPool.swap(amount); + } +} +``` + +Summary of PoC Execution: +The attacker monitors the liquidation transaction and executes a manipulation of the price oracle to influence the outcome. +The attacker ensures that their transaction is executed before the liquidation, either by paying higher gas fees or strategically timing their actions. +The liquidation is executed at an unfavorable price for the borrower, while the attacker profits from acquiring the collateral at a manipulated price. + +***Mitigation*** + +Possible Mitigation: +Commit-Reveal Mechanism: Implementing a commit-reveal mechanism can prevent the attacker from knowing the details of the liquidation transaction ahead of time. +Time-Based Locks: Enforcing a time lock on liquidation transactions can prevent attackers from front-running liquidations by forcing a delay between the transaction creation and execution. +Increase Transaction Finality: Increasing the transaction finality or using more robust price oracles (e.g., multi-source oracles) can mitigate the effect of manipulation. diff --git a/064.md b/064.md new file mode 100644 index 0000000..36592ff --- /dev/null +++ b/064.md @@ -0,0 +1,44 @@ +Expert Wool Armadillo + +Medium + +# Missing events/timelocks for admin only functions that change critical parameters + +### Summary + +Owner/admin only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages. + +### Root Cause + +Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol. +- In `LenderCommitmentGroup_Smart.sol:355-361` +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L355-L361 +This function implement minimum requirement to limit `protocolOwner` action but no event is emited +- In `LenderCommitmentGroup_Smart.sol:368-372` +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L368-L372 +This function does not implement minimum requirement/timelock to limit `owner` action and no event is emited +All of this are critical modifications and should not be done without restrictions. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +_No response_ + +### PoC + +_No response_ + +### Mitigation + +Recommend adding events to all owner/admin functions that change critical parameters. Add timelocks to introduce time delays for critical parameter changes that significantly impact the protocol users. \ No newline at end of file diff --git a/065.md b/065.md new file mode 100644 index 0000000..21a3b4c --- /dev/null +++ b/065.md @@ -0,0 +1,131 @@ +Cuddly Ginger Tadpole + +Medium + +# Fetching principal amount in active loans can revert if there were partial repayments previously + +### Summary + +The issue shows how the value of the total principal amount outstanding in active loans can revert if there were previously partial repayments made by a borrower resulting in a value of the total principal tokens repaid exceeding the total principal tokens lent and therefore in an underflow. This situation occurs when liquidating a defaulted loan. + +### Root Cause + +The root cause lies in the fact that repayments can be partial and the value of total principal tokens repaid can be increased via repay callback on these amounts. And when the loan is liquidated, this value is increased on the full principal amount of the loan again. + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +The issue occurs when a borrower makes partial repayments and then the loan is defaulted creating a situation when ` totalPrincipalTokensRepaid` > `totalPrincipalTokensLended`. + +### Impact + +The crucial function `getTotalPrincipalTokensOutstandingInActiveLoans()` would become unusable. + +### PoC + +Let's take a look at the current formula inside of the `getTotalPrincipalTokensOutstandingInActiveLoans()` function: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L712-718 +```solidity + function getTotalPrincipalTokensOutstandingInActiveLoans() + public + view + returns (uint256) + { + return totalPrincipalTokensLended - totalPrincipalTokensRepaid; + } +``` + +`totalPrincipalTokensLended` is increased when the bid is accepted and the `totalPrincipalTokensRepaid` value is increased when there are repayments made to the loan or the loan is defaulted and liquidated: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L706-707 +```solidity + //can use principal amt to increment amt paid back!! nice for math . + totalPrincipalTokensRepaid += principalAmount; +``` + +Let's say there were 300 USDT lent to a borrower and there were several partial repayments made via `TellerV2` contract resulting in a total amount repaid being 150 USDT. There is a callback that increases the principal amount: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L928-945 +```solidity + function repayLoanCallback( + uint256 _bidId, + address repayer, + uint256 principalAmount, + uint256 interestAmount + ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { + totalPrincipalTokensRepaid += principalAmount; + totalInterestCollected += interestAmount; + + emit LoanRepaid( + _bidId, + repayer, + principalAmount, + interestAmount, + totalPrincipalTokensRepaid, + totalInterestCollected + ); + } + +``` + +However, if a borrower fails to repay the remaining 150 USDT, the loan is defaulted and the liquidator would need to pay the principal amount according to this function: + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L771-781 +```solidity + function _getAmountOwedForBid(uint256 _bidId ) + internal + view + virtual + returns (uint256 amountDue) + { + (,,,, amountDue, , , ) + = ITellerV2(TELLER_V2).getLoanSummary(_bidId); + + } +``` + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L1245-1269 +```solidity + function getLoanSummary(uint256 _bidId) + external + view + returns ( + address borrower, + address lender, + uint256 marketId, + address principalTokenAddress, + uint256 principalAmount, + uint32 acceptedTimestamp, + uint32 lastRepaidTimestamp, + BidState bidState + ) + { + Bid storage bid = bids[_bidId]; + + borrower = bid.borrower; + lender = getLoanLender(_bidId); + marketId = bid.marketplaceId; + principalTokenAddress = address(bid.loanDetails.lendingToken); + principalAmount = bid.loanDetails.principal; + acceptedTimestamp = bid.loanDetails.acceptedTimestamp; + lastRepaidTimestamp = V2Calculations.lastRepaidTimestamp(bids[_bidId]); + bidState = bid.state; + } + + +``` + + +The `principalAmount` value here is the total principal amount of the vault (in our example, it's 300 USDT). This creates a situation where ` totalPrincipalTokensRepaid` becomes 450 USDT and `totalPrincipalTokensLended` remains 300 USDT resulting in a function underflow and revert. + +### Mitigation + +Make sure the `totalPrincipalTokensRepaid` is less than the amount lended by taking into account the possibility of the partial repayments. \ No newline at end of file diff --git a/066.md b/066.md new file mode 100644 index 0000000..a65d7c5 --- /dev/null +++ b/066.md @@ -0,0 +1,52 @@ +Scrawny Sapphire Baboon + +High + +# ERC2771ContextUpgradeable Missing Storage Gap + +### Summary + +The contract ERC2771ContextUpgradeable is derived from OpenZeppelin's ERC2771ContextUpgradeable library. It has been modified by removing the storage gap typically reserved at the end of the contract. This modification deviates from OpenZeppelin's standard practice for upgradeable contracts. + +### Root Cause + +OpenZeppelin's ContextUpgradeable and ERC2771ContextUpgradeable typically include a storage gap (uint256[50] private __gap;) to reserve space for future variables. +The storage gap ensures compatibility and flexibility for adding new variables in future versions of the contract. +In the provided implementation, the storage gap has been deliberately removed, as indicated by the comment: "This is modified from the OZ library to remove the gap of storage variables at the end." + +The _trustedForwarder variable is declared as immutable (address private immutable _trustedForwarder), which means its value is fixed at deployment and does not occupy storage in the contract's storage layout. +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/ERC2771ContextUpgradeable.sol#L1-L64 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Potential Impact +Upgrade Compatibility Issues: + +If this contract or any derived contracts need to be upgraded, new storage variables cannot be safely added to the contract. +Adding storage variables to the contract's layout will overwrite existing storage slots, potentially corrupting data and rendering the contract non-functional or insecure. +The removal of the storage gap deviates from OpenZeppelin's upgradeable contract design, reducing flexibility for future enhancements or bug fixes. +This increases the likelihood of human error during contract upgrades, especially when adding new variables. +The immutable _trustedForwarder cannot be changed or adjusted after deployment, which might limit adaptability to changes in the system's forwarder logic or address. + + +### PoC + +_No response_ + +### Mitigation + +```solidity +uint256[50] private __gap; +``` diff --git a/067.md b/067.md new file mode 100644 index 0000000..deb1c21 --- /dev/null +++ b/067.md @@ -0,0 +1,136 @@ +Restless Magenta Mallard + +High + +# Liquidator can acquire collateral assets for free or significantly less. + +### Summary + +Under function [liquidateDefaultedLoanWithIncentive](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L726), the liquidator can pay zero due to the following code. Look at line 730, the value to transfer can be zero if tokensToGiveToSender is equal to amountDue. + +```Solidity +File: LenderCommitmentGroup_Smart.sol +721: } else { // if minamount difference is less than or equal to zero +722: +723: +724: uint256 tokensToGiveToSender = abs(minAmountDifference);// returns 1 if x is negative 1, return zero if zero +725: +726: +727: IERC20(principalToken).safeTransferFrom( +728: msg.sender, +729: address(this), +730: amountDue - tokensToGiveToSender // this can be zero if tokenstogivetosender is equal to amount due +731: ); +``` + +Then, let's illustrate on how this can be possible. We need to first study the function of minamountdifference (see below) in which this will act as multiplier. As you can see, there is a chance this can result of -100% of amount due. Look at line 822-825. If amount due is 100, this function can result to -100. + +```Solidity +File: LenderCommitmentGroup_Smart.sol +801: function getMinimumAmountDifferenceToCloseDefaultedLoan( +802: uint256 _amountOwed, // let say 1 +803: uint256 _loanDefaultedTimestamp +804: ) public view virtual returns (int256 amountDifference_) { +805: require( +806: _loanDefaultedTimestamp > 0, +807: "Loan defaulted timestamp must be greater than zero" +808: ); +809: require( +810: block.timestamp > _loanDefaultedTimestamp, +811: "Loan defaulted timestamp must be in the past" +812: ); +813: +814: uint256 secondsSinceDefaulted = block.timestamp - +815: _loanDefaultedTimestamp; +816: +817: //this starts at 764% and falls to -100% +818: int256 incentiveMultiplier = int256(86400 - 10000) - +819: int256(secondsSinceDefaulted); +820: +821: if (incentiveMultiplier < -10000) { +822: incentiveMultiplier = -10000; +823: } +824: +825: amountDifference_ = +826: (int256(_amountOwed) * incentiveMultiplier) / +827: int256(10000); +828: } +``` + +Then let's go back again at this code snippet below, remember the result is -100 amount due for minamount difference, this can be turn positive in line 724, and then will deducted in line 730 (100-100 = 0). So the value of transfer is zero. Simply put the liquidator able to acquire collateral for free. + +```Solidity +File: LenderCommitmentGroup_Smart.sol +721: } else { // if minamount difference is less than or equal to zero +722: +723: +724: uint256 tokensToGiveToSender = abs(minAmountDifference);// returns 1 if x is negative 1, return zero if zero +725: +726: +727: IERC20(principalToken).safeTransferFrom( +728: msg.sender, +729: address(this), +730: amountDue - tokensToGiveToSender // this can be zero if tokenstogivetosender is equal to amount due +731: ); +``` + + +### Root Cause + +The root cause pertain to the formula below in line 730, it simply allows the amountdue to be reduce to zero and transfer zero tokens. The tokenstogivetosender computation should be revise also and not allow to deduct significantly to the amount due. + +```Solidity +File: LenderCommitmentGroup_Smart.sol +721: } else { // if minamount difference is less than or equal to zero +722: +723: +724: uint256 tokensToGiveToSender = abs(minAmountDifference);// returns 1 if x is negative 1, return zero if zero +725: +726: +727: IERC20(principalToken).safeTransferFrom( +728: msg.sender, +729: address(this), +730: amountDue - tokensToGiveToSender // this can be zero if tokenstogivetosender is equal to amount due +731: ); +``` + +The getMinimumAmountDifferenceToCloseDefaultedLoan computation should also be revise and not allow to fall to -100%. The allowed discount should be minimal only like 1-10% of amount due. +```Solidity +817: //this starts at 764% and falls to -100% +818: int256 incentiveMultiplier = int256(86400 - 10000) - +819: int256(secondsSinceDefaulted); +820: +821: if (incentiveMultiplier < -10000) { +822: incentiveMultiplier = -10000; +823: } +824: +``` + +### Internal pre-conditions + +1. Liquidator need to execute liquidation at the point since default in which the value of minamountdifference is significant negative value of amount due. So execution can be at range of 85,400-86,400 seconds since default time. If the execution is at exact 86,400 seconds since default, 100% guaranteed that the value transfer or payment by liquidator is zero. + +### External pre-conditions + +_No response_ + +### Attack Path +This can be the scenario. Let's say the amount due is 100 tokens. Please be reminded i did not include here the protocol fees to simplify the liquidator payment +image +1. At initial phase of liquidation, 864 is the total payment due to high multiplier +2. Decrease of multiplier , decrease also of total payment due +3. The amount difference is negative, so the else statement in liquidation function will trigger. This will give discount to total amount due. +4. If the liquidator execute liquidation at final second of 86400, which means 100% negative of amount due, therefore zero payment. + + +### Impact + +The liquidator can acquire the collateral assets for free or significantly less in which the lenders will receive nothing or very minimal in exchange. + +### PoC + +_No response_ + +### Mitigation + +Revise the formula in getMinimumAmountDifferenceToCloseDefaultedLoan which should not give significant discount to the amount due. The allowed discount should be minimal only like 1-10% of amount due. \ No newline at end of file diff --git a/068.md b/068.md new file mode 100644 index 0000000..8eef28c --- /dev/null +++ b/068.md @@ -0,0 +1,41 @@ +Petite Pewter Orangutan + +High + +# `LenderCommitmentGroupShares` missing `msg.sender` on constructor's `Ownable()` + +### Summary + +[LenderCommitmentGroupShares](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L28) missing `msg.sender` on constructor's `Ownable()` + + +### Impact + +It will not be possible to deploy the contract with the correct ownership. + +### PoC + +[LenderCommitmentGroupShares::constructor](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L28) + +```solidity + constructor(string memory _name, string memory _symbol, uint8 _decimals) + ERC20(_name, _symbol) +@> Ownable() + { + DECIMALS = _decimals; + } +``` + +### Mitigation + +Add `msg.sender` on the constructor's `Ownable()` + +```diff + constructor(string memory _name, string memory _symbol, uint8 _decimals) + ERC20(_name, _symbol) +- Ownable() ++ Ownable(msg.sender) + { + DECIMALS = _decimals; + } +``` \ No newline at end of file diff --git a/069.md b/069.md new file mode 100644 index 0000000..1c38483 --- /dev/null +++ b/069.md @@ -0,0 +1,47 @@ +Petite Pewter Orangutan + +High + +# `TellerV2::setProtocolPausingManager` missing `access control modifier` + +### Summary + +[TellerV2::setProtocolPausingManager](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L256C1-L262C6) missing access control modifier + +### Root Cause + +_No response_ + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Because it has `reinitializer` so it can be called again and because it lacks an access control modifier an attacker can set him as `_protocolPausingManager` + +### PoC + +[TellerV2::setProtocolPausingManager](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L256C1-L262C6) + +```solidity + function setProtocolPausingManager( + address _protocolPausingManager + ) external reinitializer(10) { + + _setProtocolPausingManager(_protocolPausingManager); + + } +``` + +### Mitigation + +Add the correct access control modifier to it. \ No newline at end of file diff --git a/070.md b/070.md new file mode 100644 index 0000000..5dd423c --- /dev/null +++ b/070.md @@ -0,0 +1,97 @@ +Petite Pewter Orangutan + +High + +# `TellerV2::constructor` is not calling `__Pausable_init()` which will interrupt contract's pause functionality + +### Summary + +[TellerV2::constructor](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L203) is not calling `__Pausable_init()` which will interrupt contract's pause functionality + +### Root Cause + +_No response_ + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +If you don't call `__Pausable_init()` in the constructor, the Pausable functionality won't be initialized. This means that the Pausable modifier won't work as intended + +### PoC + +[TellerV2::constructor](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L203) + +```solidity + function initialize( + uint16 _protocolFee, + address _marketRegistry, + address _reputationManager, + address _lenderCommitmentForwarder, + address _collateralManager, + address _lenderManager, + address _escrowVault, + address _protocolPausingManager + ) external initializer { + __ProtocolFee_init(_protocolFee); + +@> //__Pausable_init(); + + require( + _lenderCommitmentForwarder.isContract(), + "LCF_ic" + ); + lenderCommitmentForwarder = _lenderCommitmentForwarder; + + require( + _marketRegistry.isContract(), + "MR_ic" + ); + marketRegistry = IMarketRegistry(_marketRegistry); + + require( + _reputationManager.isContract(), + "RM_ic" + ); + reputationManager = IReputationManager(_reputationManager); + + require( + _collateralManager.isContract(), + "CM_ic" + ); + collateralManager = ICollateralManager(_collateralManager); + + + + require( + _lenderManager.isContract(), + "LM_ic" + ); + lenderManager = ILenderManager(_lenderManager); + + + + + require(_escrowVault.isContract(), "EV_ic"); + escrowVault = IEscrowVault(_escrowVault); + + + + + _setProtocolPausingManager(_protocolPausingManager); + } +``` + +### Mitigation + +Uncomment the `__Pausable_init()` \ No newline at end of file diff --git a/071.md b/071.md new file mode 100644 index 0000000..91cc86e --- /dev/null +++ b/071.md @@ -0,0 +1,72 @@ +Dandy Caramel Tortoise + +Medium + +# EMI calculation is flawed + +### Summary + +Taking min when calculating EMI repayment amount is flawed + +### Root Cause + +The amount due in case of EMI repayment is calculated as: +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/libraries/V2Calculations.sol#L124-L138 +```solidity + } else { + // Default to PaymentType.EMI + // Max payable amount in a cycle + // NOTE: the last cycle could have less than the calculated payment amount + + + //the amount owed for the cycle should never exceed the current payment cycle amount so we use min here + uint256 owedAmountForCycle = Math.min( ((_bid.terms.paymentCycleAmount * owedTime) ) / + _paymentCycleDuration , _bid.terms.paymentCycleAmount+interest_ ) ; + + + uint256 owedAmount = isLastPaymentCycle + ? owedPrincipal_ + interest_ + : owedAmountForCycle ; + + + duePrincipal_ = Math.min(owedAmount - interest_, owedPrincipal_); + } +``` + +This is incorrect and leads to lowered payments since `_bid.terms.paymentCycleAmount+interest_` will be taken instead of the ratio wise amount + +Eg: +Principal (P) = 100 +Annual Rate (r) = 12% = 0.12 +Number of Monthly Payments (n) = 12 +monthly EMI = 8.84 + +But if the repayment occurs after 2 months, this formula calculates the amount due as 8.84 + 2 == 10.84 instead of 8.84 * 2 + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +Incorrectly lowered payments in case of EMI repayments + +### PoC + +_No response_ + +### Mitigation + +Dont take the min. Instead use +```solidity + ((_bid.terms.paymentCycleAmount * owedTime) ) / + _paymentCycleDuration +``` \ No newline at end of file diff --git a/072.md b/072.md new file mode 100644 index 0000000..07b8206 --- /dev/null +++ b/072.md @@ -0,0 +1,87 @@ +Wild Chili Nuthatch + +High + +# Unchecked repayLoanCallback Execution + +***Summary*** +[https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L60-L71](url) + +***Root Cause*** +The root cause of the state inconsistency issue in the _afterTokenTransfer() function is that it resets the poolSharesPreparedToWithdrawForLender[from] and poolSharesPreparedTimestamp[from] mappings whenever tokens are transferred. This behavior is triggered for all transfers, regardless of whether the transfer is related to share burning or not. + +Detailed Explanation: +State Reset on Every Transfer: The function is designed to reset the prepared shares for the from address whenever a token transfer occurs. This includes transfers that are not related to share burns, which is problematic. + +Unintended Consequences: A user might prepare their shares for burning (by calling prepareSharesForBurn()) and then transfer tokens for reasons unrelated to share burns (such as a normal token transfer). In such cases, the prepared shares and the timestamp for burning will be incorrectly reset, potentially locking the user out of burning their shares or leading to state inconsistencies. + +Inconsistent Behavior: The poolSharesPreparedToWithdrawForLender and poolSharesPreparedTimestamp mappings are intended to track shares that are prepared for burning, but because the _afterTokenTransfer() function resets them on every transfer, it can cause confusion or unintended loss of state, especially in scenarios where a user makes transfers before burning. + +***PoC*** +To demonstrate the vulnerability, we will follow these steps: + +Initial Setup: + +The user prepares shares for burning using the prepareSharesForBurn() function. +The user has a certain amount of shares prepared to withdraw. +Transfer of Tokens: + +The user transfers some tokens, triggering the _afterTokenTransfer() function. +Check Prepared Shares: + +After the transfer, we check if the prepared shares and timestamp have been reset incorrectly. +PoC Steps: +User prepares shares for burning: +```solidity + +LenderCommitmentGroupShares sharesContract = LenderCommitmentGroupShares(address_of_contract)); + +// User prepares shares for burning +uint256 amountToPrepare = 1000; // Example amount +sharesContract.prepareSharesForBurn(user_address, amountToPrepare); +``` + +At this point, the poolSharesPreparedToWithdrawForLender[user_address] will be set to 1000, and the timestamp will be recorded. + +User transfers tokens: + +```solidity + +// User transfers tokens to another address +uint256 amountToTransfer = 100; // Example transfer amount +ERC20(token_address).transfer(other_user_address, amountToTransfer); +``` +Check Prepared Shares: +After the transfer, we check the values of poolSharesPreparedToWithdrawForLender[user_address] and poolSharesPreparedTimestamp[user_address]. + +```solidity + +uint256 preparedShares = sharesContract.poolSharesPreparedToWithdrawForLender(user_address); +uint256 preparedTimestamp = sharesContract.poolSharesPreparedTimestamp(user_address); +``` + +// These values should remain intact, but they will be reset due to _afterTokenTransfer if not handled correctly +Expected Result (Without Fix): +The state values poolSharesPreparedToWithdrawForLender[user_address] and poolSharesPreparedTimestamp[user_address] will be reset to zero due to the _afterTokenTransfer() logic, even though the transfer was not related to share burning. + +Impact: +The user will no longer be able to burn their shares as the prepared state was reset unexpectedly, causing an inconsistency in the contract's state. + +***Impact*** + +The primary impact of the vulnerability in the _afterTokenTransfer() function is state inconsistency, which can lead to unintended consequences for users. Specifically, if a user prepares shares for burning and later transfers those shares, the prepared state is reset incorrectly. Here are the key consequences: + +Inability to Burn Shares: + +If a user transfers tokens, the contract will reset the prepared shares and the timestamp even though the transfer was not related to burning shares. +As a result, the user's ability to burn the prepared shares could be blocked because the system no longer tracks the prepared shares properly. The transfer of tokens should not affect the prepared withdrawal state, but in this case, it does. +Loss of Prepared State: + +Users who prepare shares for burning may inadvertently lose their right to withdraw those shares after performing a normal transfer of tokens. +This leads to confusion and poor user experience, as users may not be aware that their shares were reset due to a non-relevant transfer. +Potential for Exploitation: + +If the prepared shares are reset incorrectly, malicious actors could take advantage of this by transferring shares to manipulate the state and prevent other users from burning their shares. Although this may be difficult to exploit directly, it creates uncertainty and a potential attack surface for future issues. +Inconsistent Accounting: + +The contract keeps track of shares that are prepared for burning using mappings like poolSharesPreparedToWithdrawForLender. Incorrect resets of these values may result in the wrong accounting, which could affect overall contract operations and the accuracy of the burn process. \ No newline at end of file diff --git a/073.md b/073.md new file mode 100644 index 0000000..accc2b0 --- /dev/null +++ b/073.md @@ -0,0 +1,179 @@ +Brisk Sapphire Chipmunk + +High + +# Missing ERC1155 Token ID Handling in Withdrawal Logic Leading to inability to withdraw collateral + +### Summary + +The withdraw function in the CollateralEscrowV1 contract fails to account for the token ID when withdrawing ERC1155 tokens. While the function supports generic collateral withdrawals, it lacks logic to handle ERC1155 tokens that require both a token address and a token ID to uniquely identify a specific asset type. + +This poses a serious issue because the protocol supports ERC 1155 to be deposited as collateral as seen in the deposit function in collateralManager.sol; +```solidity +function _deposit(uint256 _bidId, Collateral memory collateralInfo) + internal + virtual + { + require(collateralInfo._amount > 0, "Collateral not validated"); + (address escrowAddress, address borrower) = _deployEscrow(_bidId); + ICollateralEscrowV1 collateralEscrow = ICollateralEscrowV1( + escrowAddress + ); + // Pull collateral from borrower & deposit into escrow + // This will revert with fee-on-transfer tokens + if (collateralInfo._collateralType == CollateralType.ERC20) { + IERC20(collateralInfo._collateralAddress).safeTransferFrom( + borrower, + address(this), + collateralInfo._amount + ); + IERC20(collateralInfo._collateralAddress).forceApprove( + escrowAddress, + collateralInfo._amount + ); + collateralEscrow.depositAsset( + CollateralType.ERC20, + collateralInfo._collateralAddress, + collateralInfo._amount, + 0 + ); + } else if (collateralInfo._collateralType == CollateralType.ERC721) { + IERC721Upgradeable(collateralInfo._collateralAddress).transferFrom( + borrower, + address(this), + collateralInfo._tokenId + ); + IERC721Upgradeable(collateralInfo._collateralAddress).approve( + escrowAddress, + collateralInfo._tokenId + ); + collateralEscrow.depositAsset( + CollateralType.ERC721, + collateralInfo._collateralAddress, + collateralInfo._amount, + collateralInfo._tokenId + ); + } else if (collateralInfo._collateralType == CollateralType.ERC1155) { + bytes memory data; + IERC1155Upgradeable(collateralInfo._collateralAddress) + .safeTransferFrom( + borrower, + address(this), + collateralInfo._tokenId, + collateralInfo._amount, + data + ); + IERC1155Upgradeable(collateralInfo._collateralAddress) + .setApprovalForAll(escrowAddress, true); + collateralEscrow.depositAsset( + CollateralType.ERC1155, + collateralInfo._collateralAddress, + collateralInfo._amount, + collateralInfo._tokenId + ); + } else { + revert("Unexpected collateral type"); + } + emit CollateralDeposited( + _bidId, + collateralInfo._collateralType, + collateralInfo._collateralAddress, + collateralInfo._amount, + collateralInfo._tokenId + ); + } +``` + +### Root Cause + +In CollaterEscrow.sol: 85, the withdrawal logic assumes that specifying only the collateral address and amount is sufficient for all token types. This assumption breaks for ERC1155 tokens, which require a token ID to differentiate between various token types managed by the same contract. + +```solidity +function withdraw( + address _collateralAddress, + uint256 _amount, + address _recipient +) external virtual onlyOwner { + require(_amount > 0, "Withdraw amount cannot be zero"); + Collateral storage collateral = collateralBalances[_collateralAddress]; + require( + collateral._amount >= _amount, + "No collateral balance for asset" + ); + _withdrawCollateral( + collateral, + _collateralAddress, + _amount, + _recipient + ); + collateral._amount -= _amount; + emit CollateralWithdrawn(_collateralAddress, _amount, _recipient); +} +``` +The _withdrawCollateral function does not accept or process a tokenId, leaving it incapable of properly handling ERC1155 tokens. + +https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L85 + +### Internal pre-conditions + +1. The collateralBalances mapping uses the _collateralAddress as a key without differentiating between token IDs. +2. The function _withdrawCollateral lacks a parameter or logic for handling ERC1155 token IDs. + +### External pre-conditions + +1. ERC1155 tokens can represent multiple asset types (e.g., token ID 1 for "Gold" and 2 for "Silver") within the same contract. +2. Without the token ID, the contract cannot identify which ERC1155 token type to withdraw, leading to potential errors or unintended behavior. +3. Users may deposit ERC1155 tokens as collateral using the _deposit function but find themselves unable to withdraw them correctly due to the missing token ID logic. + +### Attack Path + +_No response_ + +### Impact + +1. Improper handling of ERC1155 tokens can result in asset loss. This leads to inability to withdraw collateral +2. Protocol Non-Compliance: The withdrawal function deviates from the ERC1155 standard, potentially breaking integrations + +### PoC + +1. A user deposits an ERC1155 token as collateral (e.g., token ID = 1). +2. The user(lender) or protocol attempts to withdraw the collateral using the withdraw function. +3. The function fails or withdraws an unintended token because the token ID is not specified or handled. +4. The user cannot recover their specific collateral. + +### Mitigation + +Update the withdraw Function: +Add a uint256 _tokenId parameter to the withdraw function and propagate it to _withdrawCollateral and any relevant logic. + +```solidity +function withdraw( + address _collateralAddress, + uint256 _amount, + uint256 _tokenId, + address _recipient +) external virtual onlyOwner { + require(_amount > 0, "Withdraw amount cannot be zero"); + Collateral storage collateral = collateralBalances[_collateralAddress][_tokenId]; + require( + collateral._amount >= _amount, + "No collateral balance for asset" + ); + _withdrawCollateral( + collateral, + _collateralAddress, + _amount, + _tokenId, + _recipient + ); + collateral._amount -= _amount; + emit CollateralWithdrawn(_collateralAddress, _amount, _tokenId, _recipient); +} +``` + +AND +Update the withdraw Function: +Add a uint256 _tokenId parameter to the withdraw function and propagate it to _withdrawCollateral and any relevant logic. + +```solidity +mapping(address => mapping(uint256 => Collateral)) collateralBalances; \ No newline at end of file diff --git a/invalid/.gitkeep b/invalid/.gitkeep new file mode 100644 index 0000000..e69de29