0xadrii - Not transferring collateral when submitting bids allows malicious users to create honeypot-style attacks
Opened this issue · 9 comments
0xadrii
high
Not transferring collateral when submitting bids allows malicious users to create honeypot-style attacks
Summary
Currently, Teller does not require collateral to be transferred when a bid request with collateral is submitted. Instead, the collateral is pulled from the borrower when the bid is accepted by the lender in a different transaction. This pattern allows attackers to leverage certain collaterals to perform honeypot-style attacks.
Vulnerability Detail
The current flow to create a bid in Teller putting some collateral consists in the following steps:
-
Call
submitBidwith an array ofCollateral. This creates the bid request, but does not transfer the collateral to Teller. Instead, Teller only check that Teller performs is checking the collateral balance of the borrower to guarantee that he actually owns the collateral assets. -
After submitting the bid request, an interested lender can lend his assets to the borrower by calling
lenderAcceptBid. This is the step where collateral will actually be transferred from the borrower, as shown in the following code snippet:// TellerV2.sol function lenderAcceptBid(uint256 _bidId) external override pendingBid(_bidId, "lenderAcceptBid") whenNotPaused returns ( uint256 amountToProtocol, uint256 amountToMarketplace, uint256 amountToBorrower ) { ... // Tell the collateral manager to deploy the escrow and pull funds from the borrower if applicable collateralManager.deployAndDeposit(_bidId); ... }
Although this pattern (checking the borrower’s collateral balance in step 1 to ensure he actually owns the assets) might look like a correct approach, it’s actually incorrect and can lead to honeypot-style attacks when specific NFTs are used as collateral.
Consider the following scenario: a malicious borrower holds a Uniswap V3 liquidity position NFT with liquidity worth 10 ETH. He decides to trick borrowers in Teller, and submits a loan request asking for only 1 ETH (a very attractive offer, given that on default, the lender will gain access to 10 ETH worth of collateral):
- Borrower creates the borrow request by calling
submitBid.submitBidthen checks and guarantees that the borrower indeed holds the Uniswap liquidity position. - After some time, a lender sees the borrow request and decides that they want to lend their assets to the borrower by calling
lenderAcceptBid, as the Uniswap liquidity position (which is worth 10 ETH) is attractive enough to cover the possibility of borrowed assets never being repaid. - The malicious borrower then frontruns the lender transaction and decreases the Uniswap liquidity position to nearly 0 (because collateral has not been transferred when submitting the bid request, the NFT owner is still the borrower, hence why liquidity can be decreased by him).
- After decreasing the Uniswap position liquidity, the lender’s
lenderAcceptBidtransaction actually gets executed. Uniswap’s liquidity position NFT gets transferred to Teller from the borrower, and the borrowed funds are transferred to the borrower.
As we can see, not transferring the collateral when the bid request is submitted can lead to these type of situations. Borrowers can easily trick lenders, making them believe that their loaned assets are backed by an NFT worth an X amount, when in reality the NFT will be worth nearly 0 when the transactions actually get executed.
Impact
High. Attackers can easily steal all the borrowed assets from lenders with no collateral cost at all, given that the collateral NFT will be worth 0 when the borrow is actually executed.
Code Snippet
Tool used
Manual Review
Recommendation
One way to mitigate this type of attack is to force users to transfer their collateral to Teller when a borrow request is submitted. This approach would easily mitigate this issue, as users won’t be able to perform any action over the collateral NFTs as they won’t be the owners. In the situation where the loan is never accepted and the bidExpirationTime is reached, borrowers should be able to withdraw their collateral assets.
request poc
Not sure if we can call this an issue, because the lender would have to take the risk before accepting such NFTs to be used as collateral. They can simply choose not to do so to avoid the exploit.
PoC requested from @0xadrii
Requests remaining: 14
Hey @nevillehuang ,
I understand your reasoning, however I believe that this must be considered as an issue due to the following reasons.
The root cause of the issue is inherently bound to the flawed submit-accept loan creation approach used in Teller. As mentioned in my report, the main problem is the process in which new loans are created.
Currently this process involves two separate steps:
1. A borrower submitting a loan request. In this step, only the collateral balance of the borrower is checked, but collateral is never transferred to the protocol. Not transferring the collateral in this step is the main root cause of the issue and what allows the attack vector I mentioned to take place, given that collateral is still held in the borrower's address until the loan is accepted by the lender.
2. A lender accepting the loan request. It is in this step where the collateral is actually transferred.
So there's an invariant here that must always hold: the collateral value before and after a lender accepts a loan should only be subject to changes derived from market conditions, and nothing else. This invariant must hold for any type of collateral supported in Teller. Usually, changing the value of a collateral is not possible because the value of the assets simply can't be modified and is given by the market. However, Uniswap liquidity positions are a type of asset whose value isn't only derived from the market, but also can actually be changed by adding or substracting liquidity to/from them. This is a risk that must be considered in Teller and can't be overlooked, given that any type of collateral is accepted in the protocol (Uniswap liquidity positions included, which are likely to be used as collateral in a real life scenario).
In addition, all loans must be accepted by lenders given the peer-to-peer nature of Teller, so, as you mentioned, they will always be taking a risk when accepting loans (the risk that anyone would incur when interacting with a DEFI protocol). However, the type of risk shown by the attack I described should not be expected by the lenders. It is extremely probable that they will be willing to take these kind of loans given that they will assume (as it should be with all collaterals) that the value of the collateral before and after accepting the loan won't change drastically and that they won't be exposed to an attack such as the one described.
Let me know if you still need the poc and I'll be happy to provide it!
This was a design decision made by leadership years ago. We opted to NOT transfer collateral in on submit bid in order to allow for the same collateral to be used across multiple loans. The recommendation to our users is to not accept collateral that can change (use a bored ape, dont use a Uni liq pool nft ) or make sure that token is wrapped (wrap the Liq Pool NFT) so it cant be attacked in that way at the last moment.
Thank you for your research and report.
Hey @ethereumdegen , I get your point. However, as per Sherlock's hierarchy of truth, this issue must be accepted.
Teller's contest page explicitly mentions that any token is supported in the protocol (from Teller's first contest): "Any tokens are acceptable and this is one area where research is needed to ensure that functionality is not taken advantage of and that assumptions are not broken in the protocol. This includes tokens (principal or collateral) which attempt to use re-entrancy to break assumptions." The only restriction is for ERC20 tokens, where only the ones accepted in Uniswap are supported.
Moreover, Teller's documentation explicitly mentions that "any ERC-721, ERC-721A (NFTs) can be used".
I understand that you don't want to fix it, but as per Sherlock's rules, this must be accepted as an issue given that both the README and documentation mention that any NFT could be used.
@ethereumdegen Was there public information stating this recommendation/design choice on teller docs?
The lender has to manually accept every loan they'd like to take. They must acknowledge all the risks that come with it. UniV3 positions do not have a stable price, hence any lender willing to take such position must know the risk that comes with it.
Even if collateral was transferred prior to bid's accept, there would still be ways for borrower to lower its value. E.g. if borrower offers a 1 WETH position as collateral in WETH/USDC pool at 1:1 price range, borrower would still be able to decrease position's value after transferring it, by simply swapping 1 USDC for it (making the collateral worth 1 USDC total in the same manner of a 'honey-pot' attack)
You are right, the lender needs to acknowledge all the risks, but these are market risks, not risks derived from a wrong smart contract implementation.
Obviously, users interacting with a lending protocol should be aware that market conditions can affect them, and that interacting with certain assets might be more risky than others. However, when the attack comes from an issue in the smart contracts you are interacting with (as the one I shown), rather than market conditions, the user must not be responsible for it. Otherwise, we could say that users interacting with a protocol that appraises Uniswap positions wrongly should be aware of the fact that these type of risks are possible.
Besides this, the honey-pot attack you mention is completely different to the attack that I reported. In your attack, an uncommon and unreasonable price range in the pool is required. This would already be a huge red flag for the borrower. On the other hand, the attack I detailed is possible with any price range in any Uniswap pool, which would give the lender a completely different sense on how legit the loan might be if a proper range is used. But even if we consider that a user would be willing to accept such a loan with a weird price range, the risk should still be mitigated instead of leaving the vulnerability in the protocol. For example, lenders could pass a minimum expected collateral value when accepting a loan. If the collateral is detected to be a Uniswap position, then the price of the position could be calculated and compared with the minimum required value. This would easily prevent these type of attacks. But saying that the vulnerability should be left in the contract is not a good way to protect the protocol.
As @ethereumdegen mentioned, they will be alerting their users recommending them to use wrappers or not interact with these types of assets. But as detailed in my comment, this should have been explicitly mentioned in the contest README/protocol documentation. However, both the contest README and documentation clearly stated that any type of NFT will be accepted as collateral, so this attack must be accepted as an issue.
Since teller intends to support all types of ERC721 and it was not explicitly mentioned that this was a design decision/known risk for such NFTs I believe it to be a valid issue. The design choice is suboptimal and does imply a loss of funds for lenders when such NFTs are utilized, since they can accept such bids with when such collateral are used (details at time when bids are accepted is accurate). This is exacerbated by the fact that teller has no value-based liquidation in place.