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

Incorrect authority check in `settleAskTaker`

0xbrivan2

Summary

The settleAskTaker function incorrectly enforces the bid-offer maker, instead of the taker, to settle points sold in bid offers. This misimplementation prevents proper settlement by the takers.

Vulnerability Details

Takers of bid-offers are required to settle the points they sold to bid-offer makers following the token generation event (TGE). They use the settleAskTaker function to complete this settlement. However, the current implementation incorrectly enforces the offer makers to settle the points themselves, which is erroneous:

function settleAskTaker(address _stock, uint256 _settledPoints) external {
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
StockInfo memory stockInfo = perMarkets.getStockInfo(_stock);
​
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
MarketPlaceInfo memory marketPlaceInfo,
MarketPlaceStatus status
) = getOfferInfo(stockInfo.preOffer); // @audit we get offer info from the pre offer of the stock
​
// ...
​
if (status == MarketPlaceStatus.AskSettling) {
if (_msgSender() != offerInfo.authority) { // @audit Wrong check
revert Errors.Unauthorized();
}
}
// ...
}
​

As shown above, the original offer is retrieved from stockInfo.preOffer, which references the bidder maker's offer. The function then incorrectly enforces the caller to be the offer maker. This is incorrect because the offer maker has purchased points from the taker, and it is the taker who should be responsible for settling the points.

Proof Of Concept

Consider the following scenario, demonstrated by the test case below:

  1. As the Original Offer Maker, Alice posts a buy offer for 1,000 points with a unit price of 1,000` in advance.

  2. Bob accepts Alice's buy offer and sells 1,000 points to Alice. This transaction results in Bob having a stock of type Ask to be settled later after TGE.

  3. During the settlement phase, Bob should settle the 1,000 points for Alice after TGE. But, the current implementation incorrectly requires Alice to settle the points she bought.

Copy and paste the following test in test/PreMarkets.sol:

import {Errors} from "../src/utils/Errors.sol";
​
function test_askTakersCanNotSettlePointsToBidOffers() public {
// As the Original Offer Maker, Alice (user) posts a buy offer for 1,000 points with a unit price of $1
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1000 * 1e18,
10000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
address offer = GenerateAddress.generateOfferAddress(0);
// Bob (user2) accepts Alice's buy offer and sells 1,000 points to Alice
vm.startPrank(user2);
preMarktes.createTaker(offer, 1000);
address user2Stock = GenerateAddress.generateStockAddress(1);
​
// Updating maker info by the owner, setting TGE time
vm.startPrank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
// Bob should settle the 1000 points to Alice now. However, he can not
vm.startPrank(user2);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
vm.expectRevert(Errors.Unauthorized.selector);
deliveryPlace.settleAskTaker(user2Stock, 1000);
}
forge test --mt test_askTakersCanNotSettlePointsToBidOffers
​
[PASS] test_askTakersCanNotSettlePointsToBidOffers() (gas: 954970)

Impact

Bid offers takers can not settle their points, which results in:

  1. Losing collateral even if the taker wanted to settle their points

  2. Bidder offers do not receive their token points

Tools Used

Manual review

Recommendations

Consider removing the mentioned offer authority check and replace it with stock authority check. Below is a suggested update to the settleAskTaker function:

function settleAskTaker(address _stock, uint256 _settledPoints) external {
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
StockInfo memory stockInfo = perMarkets.getStockInfo(_stock);
​
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
MarketPlaceInfo memory marketPlaceInfo,
MarketPlaceStatus status
) = getOfferInfo(stockInfo.preOffer);
​
// ...
​
if (status == MarketPlaceStatus.AskSettling) {
- if (_msgSender() != offerInfo.authority) {
+ if (_msgSender() != stockInfo.authority)
revert Errors.Unauthorized();
}
}
// ...
}
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.