-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
48f4599
commit 14a704d
Showing
75 changed files
with
6,130 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} | ||
} | ||
} | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Oops, something went wrong.