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

`settleAskTaker` is incorrectly enforced to be called by the maker instead of the taker

fyamf

Summary

In the function settleAskTaker, it incorrectly enforces the caller to be the maker, while the caller should be the taker.

Vulnerability Details

  • Suppose Alice is a bid maker, and she creates an offer to buy 1000 points for $1 by calling createOffer.

  • Bob is a taker against Alice's offer, and he promises to provide 1000 points to Alice in the future (when TGE happens). He creates an order by calling createTaker.

  • When the owner updates the market and TGE happens, it is possible to settle.

  • The function settleAskTaker should be called to settle this trade.

The issue is as follows:

In the function settleAskTaker, it checks that the caller is the authority of the offer which is the maker (Alice).

if (status == MarketPlaceStatus.AskSettling) {
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
}

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

If Alice (maker) calls this function, it is expected that Alice transfers marketPlaceInfo.tokenAddress into the pool.

tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);

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

This does not make sense at all. Because, Alice was the bid maker, and she intended to buy points, it means that Alice does not have any marketPlaceInfo.tokenAddress. In other words, during the settlement, it is expected that required amount of marketPlaceInfo.tokenAddress owned/promised by the taker (Bob) will be transferred to the pool, and then this amount will be added to the balance of maker (Alice).

The root cause of this issue is that the function by mistake enforces that the caller of settleAskTaker to be the maker, while it should enforce to be the taker.

PoC

In the following test, Alice (maker) creates a bid offer, and Bob (taker) creates an ask order. During the settlement, when calling the function settleAskTaker, two cases are investigated:

  • If the caller is Alice (maker), it will revert by TransferFailed, because the maker does not have any marketPlaceInfo.tokenAddress to transfer.

  • If the caller is Bob (taker), it will revert by Unauthorized, because the function enforces that the caller is the maker.

Note that, there is a test already written by the protocol that does not revert, but in that test the maker already had some marketPlaceInfo.tokenAddress, which is not the real case, and the test is not correct either.
https://github.com/Cyfrin/2024-08-tadle/blob/main/test/PreMarkets.t.sol#L226
https://github.com/Cyfrin/2024-08-tadle/blob/main/test/PreMarkets.t.sol#L117

function test_maker_bid_settlement_impossible() public {
address Alice = vm.addr(10); // maker
address Bob = vm.addr(11); // taker
​
////// Alice(maker) creates a bid offer
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
////////////////
​
///// Bob (taker) creates an ask 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();
/////////////
​
//// owner updates the market
vm.startPrank(user1);
​
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
1 * 1e18,
block.timestamp - 1,
3600
);
​
vm.stopPrank();
/////////////
​
///// Bob(taker)
vm.startPrank(Bob);
​
// after TGE, Bob (taker) receives the newly-generated tokens
// he had 1000 points, and tokenPerPoint is 1e18 ==> so he gains 1000e18 newly-generated tokens
deal(address(mockPointToken), Bob, 1000 * 10 ** 18);
​
mockPointToken.approve(address(tokenManager), type(uint256).max);
​
vm.stopPrank();
/////////////
​
//// if maker settles
vm.startPrank(Alice);
​
address stockAddr = GenerateAddress.generateStockAddress(1);
​
vm.expectRevert(abi.encodeWithSignature("TransferFailed()"));
deliveryPlace.settleAskTaker(stockAddr, 1000);
​
vm.stopPrank();
/////////////
​
//// if taker settles
vm.startPrank(Bob);
​
vm.expectRevert(abi.encodeWithSignature("Unauthorized()"));
deliveryPlace.settleAskTaker(stockAddr, 1000);
​
vm.stopPrank();
/////////////
}

Impact

  • Settlement of a bid offer is impossible.

  • Loss of fund.

  • Incorrect implementation of the code.

Tools Used

Recommendations

The following is recommended:

if (_msgSender() != stockInfo.authority) {
revert Errors.Unauthorized();
}

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

Updates

Lead Judging Commences

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

finding-PreMarkets-settleAskTaker-wrong-stock-authority

Valid high severity, when taker offers are created pointing to a `offer`, the relevant `stockInfoMap` offers are created with the owner of the offer aka `authority`, set as the creater of the offer, as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L245). Because of the wrong check within settleAskTaker, it will permanently DoS the final settlement functionality for taker offers for the maker that listed the original offer, essentially bricking the whole functionality of the market i.e. maker will always get refunded the original collateral, and takers will never be able to transact the original points put up by the maker. This occurs regardless of market mode.

Support

FAQs

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