pkqs90 - `LenderCommitmentGroup_Smart` is susceptible to donation attack
Closed this issue · 0 comments
pkqs90
medium
LenderCommitmentGroup_Smart
is susceptible to donation attack
Summary
LenderCommitmentGroup_Smart
implements a ERC4626 like share/asset concept. However, it is also susceptible to the famous ERC4626 donation (inflation) attack: attackers can donate a large amount of assets (principal tokens) quickly after initialization to inflate the asset/share ratio and steal tokens from initial depositers.
Vulnerability Detail
Please read https://docs.openzeppelin.com/contracts/5.x/erc4626 to first understand the donation attack.
To make it simple, an attack vector is given:
- Attacker deposits 100 wei principal tokens, and receives 100 wei shares
- Attacker "donates" 1e20 prinipal tokens.
- Another user deposits 1e16 principal tokens, but since the asset/share is already 1e18, the share for this user is round down to 0.
- Attacker can burn all his shares and retrieve all principal tokens, including the 1e16 tokens in step 3.
Steps 1-2 are done in 1 transaction to frontrun the victim in step 3.
Now, we show how to perform the "donation" part in LenderCommitmentGroup_Smart
contract. Since the "asset amount" is calculated by function getPoolTotalEstimatedValue()
, a simple token transfer wouldn't work here. We can do the following:
- Attacker creates a bid and loan in
TellerV2
- Attacker calls
TellerV2#claimLoanNFT
and transfers the NFT toLenderCommitmentGroup_Smart
as owner. Now, theLenderCommitmentGroup_Smart
contract is able to callTellerV2#lenderCloseLoanWithRecipient
, which meansliquidateDefaultedLoanWithIncentive
is callable. - Attacker wait until the loan is default.
- Attacker calls
liquidateDefaultedLoanWithIncentive(uint256 _bidId, int256 _tokenAmountDifference)
with a very large_tokenAmountDifference
. This will greatly increasetokenDifferenceFromLiquidations
, which is used ingetPoolTotalEstimatedValue()
.
Steps 1-3 are preparation steps, and step 4 can be used as the "donation" step in the attack.
Now, we have shown the attacker can find a way to perform "donation" in 1 transaction, and the donation attack is available.
Also, the asset/share ratio can also be increased by this attack, meaning that only large amount of asset deposits will result to non-zero shares, and retail users may not be able to participate depositing LP.
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;
}
function liquidateDefaultedLoanWithIncentive(
uint256 _bidId,
int256 _tokenAmountDifference
) public bidIsActiveForGroup(_bidId) {
uint256 amountDue = getAmountOwedForBid(_bidId, false);
uint256 loanDefaultedTimeStamp = ITellerV2(TELLER_V2)
.getLoanDefaultTimestamp(_bidId);
int256 minAmountDifference = getMinimumAmountDifferenceToCloseDefaultedLoan(
amountDue,
loanDefaultedTimeStamp
);
require(
_tokenAmountDifference >= minAmountDifference,
"Insufficient tokenAmountDifference"
);
if (_tokenAmountDifference > 0) {
//this is used when the collateral value is higher than the principal (rare)
//the loan will be completely made whole and our contract gets extra funds too
uint256 tokensToTakeFromSender = abs(_tokenAmountDifference);
IERC20(principalToken).transferFrom(
msg.sender,
address(this),
amountDue + tokensToTakeFromSender
);
tokenDifferenceFromLiquidations += int256(tokensToTakeFromSender);
totalPrincipalTokensRepaid += amountDue;
} else {
uint256 tokensToGiveToSender = abs(_tokenAmountDifference);
IERC20(principalToken).transferFrom(
msg.sender,
address(this),
amountDue - tokensToGiveToSender
);
tokenDifferenceFromLiquidations -= int256(tokensToGiveToSender);
totalPrincipalTokensRepaid += amountDue;
}
//this will give collateral to the caller
ITellerV2(TELLER_V2).lenderCloseLoanWithRecipient(_bidId, msg.sender);
}
Impact
- First depositors may be frontrun and lose funds
- Attackers may "inflate" the asset/share ratio, causing retail users with small amount of asset not be able to participate in depositing LP.
Code Snippet
- https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L307-L322
- https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L288-L302
Tool used
Manual review
Recommendation
Like what https://docs.openzeppelin.com/contracts/5.x/erc4626 did, it is best to use introduce a virtual share concept.
Duplicate of #64