nobody2018 - If the collateral is a fee-on-transfer token, repayment will be blocked
Opened this issue · 9 comments
nobody2018
medium
If the collateral is a fee-on-transfer token, repayment will be blocked
Summary
As we all know, some tokens will deduct fees when transferring token. In this way, the actual amount of token received by the receiver will be less than the amount sent. If the collateral is this type of token, the amount of collateral recorded in the contract will bigger than the actual amount. When the borrower repays the loan, the amount of collateral withdrawn will be insufficient, causing tx revert.
Vulnerability Detail
The _bidCollaterals mapping of CollateralManager records the CollateralInfo of each bidId. This structure records the collateral information provided by the user when creating a bid for a loan. A lender can accept a loan by calling TellerV2.lenderAcceptBid that will eventually transfer the user's collateral from the user address to the CollateralEscrowV1 contract corresponding to the loan. The whole process will deduct fee twice.
//CollateralManager.sol
function _deposit(uint256 _bidId, Collateral memory collateralInfo)
internal
virtual
{
......
// Pull collateral from borrower & deposit into escrow
if (collateralInfo._collateralType == CollateralType.ERC20) {
IERC20Upgradeable(collateralInfo._collateralAddress).transferFrom( //transferFrom first time
borrower,
address(this),
collateralInfo._amount
);
IERC20Upgradeable(collateralInfo._collateralAddress).approve(
escrowAddress,
collateralInfo._amount
);
collateralEscrow.depositAsset( //transferFrom second time
CollateralType.ERC20,
collateralInfo._collateralAddress,
collateralInfo._amount, //this value is from user's input
0
);
}
......
}The amount of collateral recorded by the CollateralEscrowV1 contract is equal to the amount originally submitted by the user.
When the borrower repays the loan, collateralManager.withdraw will be triggered. This function internally calls CollateralEscrowV1.withdraw. Since the balance of the collateral in the CollateralEscrowV1 contract is less than the amount to be withdrawn, the entire transaction reverts.
//CollateralEscrowV1.sol
function _withdrawCollateral(
Collateral memory _collateral,
address _collateralAddress,
uint256 _amount,
address _recipient
) internal {
// Withdraw ERC20
if (_collateral._collateralType == CollateralType.ERC20) {
IERC20Upgradeable(_collateralAddress).transfer( //revert
_recipient,
_collateral._amount //_collateral.balanceOf(address(this)) < _collateral._amount
);
}
......
}Impact
The borrower's collateral is stuck in the instance of CollateralEscrowV1. Non-professional users will never know that they need to manually transfer some collateral into CollateralEscrowV1 to successfully repay.
- This issue blocked the user's repayment, causing the loan to be liquidated.
- The liquidator will not succeed by calling
TellerV2.liquidateLoanFull.
Code Snippet
Tool used
Manual Review
Recommendation
Two ways to fix this issue.
- The
afterBalance-beforeBalancemethod should be used when recording the amount of collateral. -
--- a/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol +++ b/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol @@ -165,7 +165,7 @@ contract CollateralEscrowV1 is OwnableUpgradeable, ICollateralEscrowV1 { if (_collateral._collateralType == CollateralType.ERC20) { IERC20Upgradeable(_collateralAddress).transfer( _recipient, - _collateral._amount + IERC20Upgradeable(_collateralAddress).balanceOf(address(this)) ); }
This is the same as the issue that I explained in the readme for this contest about 'poisoned collateral'. It has been known previously that collateral in the escrow could be made non-transferrable which makes loan repayment impossible. Thank you for the report. Thisis a re-iteration of what was stated as 'known issues' in the contest readme.
Escalate for 10 USDC
Disagree with sponsor's comment. This is not the case described in the Readme. The Readme states:
A: Known issue 1: Collateral assets that can be 'paused' for transfer do exhibit a risk since they may not be able to be withdrawn from the loans as collateral. Furthermore, collateral assets that can be made non-transferrable can actually 'poison' a collateral vault and make a loan non-liquidateable since a liquidateLoan call would revert.
The contest FAQ states:
FEE-ON-TRANSFER: any
The project clearly hasn't implemented logic for fee-on-transfer tokens when it has clearly stated that it should be able to operate with such tokens.
Would love to hear judge's opinion on this.
Escalate for 10 USDC
Disagree with sponsor's comment. This is not the case described in the Readme. The Readme states:A: Known issue 1: Collateral assets that can be 'paused' for transfer do exhibit a risk since they may not be able to be withdrawn from the loans as collateral. Furthermore, collateral assets that can be made non-transferrable can actually 'poison' a collateral vault and make a loan non-liquidateable since a liquidateLoan call would revert.
The contest FAQ states:
FEE-ON-TRANSFER: any
The project clearly hasn't implemented logic for fee-on-transfer tokens when it has clearly stated that it should be able to operate with such tokens.
Would love to hear judge's opinion on this.
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
According to README.md:
Collateral should not be able to be permanently locked/burned in the contracts.
So this issue is valid M.
yeah, i second it's a valid M.
Let's see the judges opinion tho
Escalation accepted
Valid medium
This issue is a valid medium as fee-on-transfer tokens are in scope.
Escalation accepted
Valid medium
This issue is a valid medium as fee-on-transfer tokens are in scope.
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
Fix looks good. Repaying a loan in full no longer forces withdrawing collateral, preventing the repay call from reverting