Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Inconsistent Authority Check in `PreMarkets::abortBidTaker` Function

pro_king

Description

The PreMarkets::abortBidTaker function currently checks whether the msg.sender (the caller of the function) is the authority of the stock (stockInfo.authority). However, according to the function's NatSpec, only the offer owner (offerInfo.authority) should be able to abort a bid taker. This inconsistency allows the stock owner to abort the bid, but not the actual offer owner, as intended by the function's design.

/**
* @notice abort bid taker
* @param _stock stock address
* @param _offer offer address
@>>> * @notice Only offer owner can abort bid taker
* @dev Only offer abort status is aborted can be aborted
* @dev Update stock authority refund amount
*/
function abortBidTaker(address _stock, address _offer) external {
StockInfo storage stockInfo = stockInfoMap[_stock];
OfferInfo storage preOfferInfo = offerInfoMap[_offer];
​
@>>> if (stockInfo.authority != _msgSender()) {
revert Errors.Unauthorized();
}
​
if (stockInfo.preOffer != _offer) {
revert InvalidOfferAccount(stockInfo.preOffer, _offer);
}
​
if (stockInfo.stockStatus != StockStatus.Initialized) {
revert InvalidStockStatus(StockStatus.Initialized, stockInfo.stockStatus);
}
​
if (preOfferInfo.abortOfferStatus != AbortOfferStatus.Aborted) {
revert InvalidAbortOfferStatus(AbortOfferStatus.Aborted, preOfferInfo.abortOfferStatus);
}
​
uint256 depositAmount = stockInfo.points.mulDiv(preOfferInfo.points, preOfferInfo.amount, Math.Rounding.Floor);
uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType, preOfferInfo.collateralRate, depositAmount, false, Math.Rounding.Floor
);
​
MakerInfo storage makerInfo = makerInfoMap[preOfferInfo.maker];
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(TokenBalanceType.MakerRefund, _msgSender(), makerInfo.tokenAddress, transferAmount);
​
stockInfo.stockStatus = StockStatus.Finished;
​
emit AbortBidTaker(_offer, _msgSender());
}
​

Impact

The impact of this inconsistency is two fold:

  1. Security Risk: Unauthorized abort actions can occur if the stock owner (stockInfo.authority) is different from the offer owner (offerInfo.authority). This could lead to scenarios where bid takers are unfairly aborted by an entity not intended to have that authority.

  2. Business Logic Violation: The intended logic of the contract, where only the offer owner should have the ability to abort a bid taker, is violated. This could lead to unexpected behavior, trust issues with users, and potential financial losses if the wrong party aborts bids.

Tools Used

Manual Review

Recommendations

To mitigate this issue, modify the authority check in the PreMarkets::abortBidTaker function to ensure that the msg.sender is the offer owner (offerInfo.authority) instead of the stock owner (stockInfo.authority). This aligns with the function's intended logic and ensures that only the rightful authority has the power to abort a bid taker.

- if (stockInfo.authority != _msgSender()) {
+ if (offerInfo.authority != _msgSender()) {
revert Errors.Unauthorized();
}
Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-PreMarkets-abortBidTaker-wrong-stock-authority

Invalid. 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). Hence, we should verify `stockInfoMap`, regardless of the taker order being a ASK (selling points) or BID (buying points) taker order, so there is no issue here, other than documentation error

Support

FAQs

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