samuraii77 - A market owner can put borrowers in a very unfavorable position and steal money out of lenders
Closed this issue · 1 comments
samuraii77
high
A market owner can put borrowers in a very unfavorable position and steal money out of lenders
Summary
A market owner can put borrowers in a very unfavorable position and steal money out of lenders
Vulnerability Detail
Users can create their own markets with their own terms. Then, lenders and borrowers can use that market to have different deals. However, a market owner can easily steal money out of the borrowers and leave them in an extremely unfavorable situation, leave them without any principal to take but with the obligation to pay back the loan which means that they can either pay it out even though they did not receive any principal amount or decide to not pay it and have their collateral taken as well as have their account reputation marked by the system.
The scenario that I will explain will be in a case where a lender and a market owner have teamed up (they can even be the same person). Another possible scenario doesn't include a lender on the side of the market owner but just the market owner frontrunning.
Imagine the following scenario (Bob
is the market owner, Alice
is the borrower, Eve
is the lender:
Bob
creates his own market with very favorable terms for borrowers and lenders usingMarketRegistry::createMarket()
Alice
submits a bid to borrow funds usingBob
's market as she likes the termsBob
has set usingTellerV2::submitBid()
(the one with theCollateral[]
variable included). Let's sayAlice
wants a principal of 1000 tokens and has given 1500 tokens as collateral.Bob
andEve
want to stealAlice
's collateral.Bob
calls thesetMarketFeePercent()
function and sets the new market fee to a value of100% - protocol fee percent
usingsetMarketFeePercent()
. Let's say the protocol fee is 10% so the new market fee is 90%.
function setMarketFeePercent(uint256 _marketId, uint16 _newPercent)
public
ownsMarket(_marketId)
{
require(_newPercent >= 0 && _newPercent <= 10000, "invalid percent");
if (_newPercent != markets[_marketId].marketplaceFeePercent) {
markets[_marketId].marketplaceFeePercent = _newPercent;
emit SetMarketFee(_marketId, _newPercent);
}
}
- Then
Eve
accepts the bid created byAlice
.
amountToProtocol = bid.loanDetails.principal.percent(protocolFee());
amountToMarketplace = bid.loanDetails.principal.percent(
marketRegistry.getMarketplaceFee(bid.marketplaceId)
);
amountToBorrower =
bid.loanDetails.principal - amountToProtocol - amountToMarketplace;
//transfer fee to protocol
if (amountToProtocol > 0) {
bid.loanDetails.lendingToken.safeTransferFrom(
sender,
owner(),
amountToProtocol
);
}
//transfer fee to marketplace
if (amountToMarketplace > 0) {
bid.loanDetails.lendingToken.safeTransferFrom(
sender,
marketRegistry.getMarketFeeRecipient(bid.marketplaceId),
amountToMarketplace
);
}
//transfer funds to borrower
if (amountToBorrower > 0) {
bid.loanDetails.lendingToken.safeTransferFrom(
sender,
bid.receiver,
amountToBorrower
);
}
This is a small snippet out of lenderAcceptBid()
used for accepting bids created by borrowers. First of all, it takes the amountToProtocol
out of the principal, that is 10% out of the 1000 tokens principal, so 100 tokens. Then, the fee for the marketplace will be 90% as that was just set by Bob
and the protocol doesn't use a cached value of the fee but instead fetches it at that very moment. The amountToMarketplace
will be 90% out of 1000 tokens, 900 tokens. amountToBorrower
will be 1000 - 100 - 900, so 0 tokens. Then, the fees for the protocol are transferred to them, the amount for the marketplace is sent to Bob
or someone Bob
has set to receive the fees and Alice
receives a grand total of 0 tokens. However, she has still deposited 1500 tokens as collateral to an escrow contract.
// Tell the collateral manager to deploy the escrow and pull funds from the borrower if applicable
collateralManager.deployAndDeposit(_bidId);
She also still has 1000 tokens as principal on paper however as seen above, she actually got 0. That means that she either has to pay that principal amount + interest or her loan will default and she will have her collateral taken leaving her in an extremely unfavourable situation. In the end, Eve
and Bob
used 100 tokens (100 for the protocol while the 900 get sent to a trusted address) and they got 1000 tokens + interest if Alice
pays out the loan or 1500 tokens in collateral if she doesn't. The other possible scenario is the market owner just frontrunning the acceptance of the bid causing a situation where the lender is in a neutral situation, he paid the tokens to the protocol and the market but he will still receive the interest + tokens or the collateral and the borrower is in the same unpleasant situation.
Impact
A market owner can steal money out of borrowers
Code Snippet
https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/MarketRegistry.sol#L636-L645
https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L524-L558
Tool used
Manual Review
Recommendation
Cache the market fee at the time of bid submission.
Duplicate of #125
@nevillehuang I believe this issue should be a high.
As per docs, the following is a necessary for an issue to be classified as a high:
Definite loss of funds without (extensive) limitations of external conditions.
The issue I described has a definite loss of funds (borrowers will either have to repay money that they haven't received or they will lose their collateral, clear loss of funds in both cases) and is achievable with little to no external conditions (a market owner who can be any user just has to change the fee to 100% - protocol fee percent
). Issue should clearly be a high.