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

There is an incorrect permission check in the `DeliveryPlace::settleAskTaker()` function, where the caller's address is mistakenly validated against the wrong authority.

joicygiore

Summary

There is an incorrect permission check in the DeliveryPlace::settleAskTaker() function, where the caller's address is mistakenly validated against the wrong authority.

Vulnerability Details

The issue arises during the transaction process that begins with UserA creating a Bid.offer. The process is as follows:

  1. UserA creates a Bid.offer by calling PreMarkets::createOffer().

  2. UserB calls PreMarkets::createTaker() using the Bid.offer.offerAddr and a specified amount.

  3. The administrator updates the market by calling SystemConfig::updateMarket.

  4. UserB then calls DeliveryPlace::settleAskTaker() to settle the transaction. During this step, UserB transfers mockPointToken to the contract, fulfilling the amount promised in step 2, and updates the balance information in userTokenBalanceMap.

Let's review the relevant section of the DeliveryPlace::settleAskTaker() function:

/**
* @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 {
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
StockInfo memory stockInfo = perMarkets.getStockInfo(_stock);
​
​
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
MarketPlaceInfo memory marketPlaceInfo,
MarketPlaceStatus status
@> ) = getOfferInfo(stockInfo.preOffer);
​
​
// SNIP...
​
​
if (status == MarketPlaceStatus.AskSettling) {
@> if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
} else {
if (_msgSender() != owner()) {
revert Errors.Unauthorized();
}
if (_settledPoints > 0) {
revert InvalidPoints();
}
}
​
​
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint *
_settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
@> tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);
​
​
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
@> offerInfo.authority,
makerInfo.tokenAddress,
settledPointTokenAmount
);
}
​
// SNIP...
}

The permission check marked by the @> symbol verifies that the caller's address is equal to offerInfo.authority, which is the address of UserA, the creator of the Bid.offer. However, this check is incorrect because it should be verifying against stockInfo.authority, which is more appropriate given the context of the function.

Poc

To demonstrate the issue, add the following test code to test/PreMarkets.t.sol and run it:

import {Errors} from "../src/utils/Errors.sol";
function test_DeliveryPlace_settleAskTaker_sender_check_error() public {
///////////////////////////
// user create Bid.Offer //
///////////////////////////
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
// Cache user's offer address
address offerAddr = GenerateAddress.generateOfferAddress(0);
////////////////////////
// user2 create Taker //
////////////////////////
vm.prank(user2);
preMarktes.createTaker(offerAddr, 1000);
​
// Cache user2's stock address
address user2StockAddr = GenerateAddress.generateStockAddress(1);
​
////////////////////////
// admin updateMarket //
////////////////////////
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
//////////////////////////
// user2 settleAskTaker //
//////////////////////////
vm.startPrank(user2);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
// The call was reverted due to a permission error.
vm.expectRevert(Errors.Unauthorized.selector);
deliveryPlace.settleAskTaker(user2StockAddr, 1000);
vm.stopPrank();
}
// Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
// [PASS] test_DeliveryPlace_settleAskTaker_sender_check_error() (gas: 955327)

Code Snippet

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

Impact

There is an incorrect permission check in the DeliveryPlace::settleAskTaker() function, where the caller's address is mistakenly validated against the wrong authority.

Tools Used

Manual Review

Recommendations

Consider updating your code as follows to correct the permission check:

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);
​
​
// SNIP...
​
​
if (status == MarketPlaceStatus.AskSettling) {
- if (_msgSender() != offerInfo.authority) {
+ if (_msgSender() != stockInfo.authority) {
revert Errors.Unauthorized();
}
} else {
if (_msgSender() != owner()) {
revert Errors.Unauthorized();
}
if (_settledPoints > 0) {
revert InvalidPoints();
}
}
​
​
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint *
_settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);
​
​
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
offerInfo.authority,
makerInfo.tokenAddress,
settledPointTokenAmount
);
}
​
// SNIP...
}
Updates

Lead Judging Commences

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

Give us feedback!