Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

The settleAskTaker function is being settle by the wrong authority

mikebello

Summary

The settleAskTaker function should be calleable by the stock authority (Taker) when the TGE has passed an the market is in AskSettling status or by the owner of the contract otherwise, but currently, this function allows the offer authority (Maker) to settle an Ask Taker order.

Vulnerability Details

When the TGE has passed the market enter the AskSettling status, in this state, all Ask Maker and all Ask Taker orders can be settled by calling the function settleAskMaker and settleAskTaker respectively, base on the code and the natspec documentation of the code, the offer authority is in charge of settle his own Ask Maker orders by calling the settleAskMaker function, and the stock authority should be in charge of settling his Ask Taker orders by calling the settleAskTaker function, but this last statement is not implemented correctly in the current code.

As you can see in the next code and in its comment, the offerInfo.authority can settle his ask maker order when the market is in AskSettling status, if the market is in another state only the owner of the contract can settle an ask maker order.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L214-L325

/**
* @notice Settle ask maker
* @dev caller must be offer authority
* @dev offer status must be Virgin or Canceled
* @dev market place status must be AskSettling
* @param _offer offer address
* @param _settledPoints settled points
*/
function settleAskMaker(address _offer, uint256 _settledPoints) external
{
// Code Omitted.
if (status == MarketPlaceStatus.AskSettling) {
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
} else {
if (_msgSender() != owner()) {
revert Errors.Unauthorized();
}
if (_settledPoints > 0) {
revert InvalidPoints();
}
}
// Code Omitted.
}

But in the settleAskMaker function the stock authority should be in charge of settling his Ask Taker order when the market is in AskSettling status, as the comment says, but instead the offer authority is in charge, which is incorrect and should be corrected, so the correct authority has the settlement right over his ask taker order.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L327-L433

/**
* @notice Settle ask taker
* @dev caller must be stock authority
* @dev market place status must be AskSettling
* @param _stock stock address
* @param _settledPoints settled points
* @notice _settledPoints must be less than or equal to stock points
*/
function settleAskTaker(address _stock, uint256 _settledPoints) external {
//... Code Omitted
​
if (status == MarketPlaceStatus.AskSettling) {
// Incorrect authority has the right to settle the Ask Taker Order.
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
} else {
if (_msgSender() != owner()) {
revert Errors.Unauthorized();
}
if (_settledPoints > 0) {
revert InvalidPoints();
}
}
//... Code Omitted
}

To show that the stock authority is not allowed to settle his own Ask Taker Order, I added this test to the PreMarkets.t.sol contract, as you can see the test revert when it's called by the stock authority user.

function test_StockAuthority_Cant_Settle_AskTaker_Order() public {
vm.startPrank(user);
// user creates an bid maker order. (offer authority)
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 0.01 * 1e18, 12000, 300, OfferType.Bid, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), 100000 * 10 ** 18);
// User1 create an Ask Taker order (stock authority), to fullfil the maker order.
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
​
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
vm.stopPrank();
​
vm.startPrank(user1);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
address stock1Addr = GenerateAddress.generateStockAddress(1);
​
// User1 (stock authority) tries to settle his Ask Taker Order, but it reverts.
vm.expectRevert(Errors.Unauthorized.selector);
deliveryPlace.settleAskTaker(stock1Addr, 500);
vm.stopPrank();
}

Impact

The settleAskTaker function is being settle by the wrong authority, this allow the maker to be in charge of the settlement instead of the taker who should be the entity in charge of the settlement.

Tools Used

Manual Code Review.

Recommendations

Change the offerInfo.authority for the stockInfo.authority in the settleAskTaker function, so the correct authority is in charge of settle the Ask Taker Order.

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

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months 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.