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

Makers are unable to settle remaining offer points during partial settlement

OdeWeb3

Summary

The settleAskMaker is a function present in DeliveryPlace which is used to Ask offers by Makers. However, the current logic is susceptible to being broken and reverts on subsequent calls when called by a Maker - a critical invariant.

Line Of Code

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

Description

Tadle is a protocol that allows users to trade points(buy or sell) and can be done by Makers who create offers that are then being matched by corresponding Takers interested in the offer created by the Maker.

After the Makers and Takers have been matched by the prospective offerId, there comes a time for settlements which happens in the DeliveryPlace::settleAskMaker.
The settleAskMaker does some checks and ensure that status == MarketPlaceStatus.AskSettling before proceeding to makes a call to perMarkets.settledAskOffer which is a function only callable by onlyDeliveryPlace.

The DeliveryPlace::settleAskMaker updates the state of the offerInfo.offerStatus to OfferStatus.Settled even in situations where offers were meant to be partially settled.

The bug lies in the fact that when a Maker settles some portions of the point, the system gets bricked and no longer able to settle the remaining points to a user due the status change not properly being handled.

code Snippet

function settledAskOffer(
address _offer,
uint256 _settledPoints,
uint256 _settledPointTokenAmount
) external onlyDeliveryPlace(tadleFactory, _msgSender()) {
OfferInfo storage offerInfo = offerInfoMap[_offer];
offerInfo.settledPoints = _settledPoints;
offerInfo.settledPointTokenAmount = _settledPointTokenAmount;
offerInfo.offerStatus = OfferStatus.Settled; //@audit this does not check if all the points have been settled before changing the state to settled. offers that were partially settled becomes bricked
​
emit SettledAskOffer(_offer, _settledPoints, _settledPointTokenAmount);
}

The above implementation breaks a core invariant of the protocol and has immerse impacts on both the protocol and the users.

ILlustration

  • Bob - a Maker, decides to offer some points(1000 points) for sale on a marketplace by depositing collateral.

  • Alice - a Taker sees this offer and Decides to buy this points(1000 points)

  • After a while vm.warp(few days), Bob decides to settle this offer partially by passing some amount of the total points (_settledPoints) that was offered to Alice.

  • Bob then make a call to DeliveryPlace::settleAskMaker with the generated offer address as _offer and 500 as _settledPoints

  • The transaction goes through. However, bob stills owes Alice settlements worth 500 points which in this case becomes bricked and can no longer be settled.

After further evaluation, it can be proven that this is not a design choice, otherwise the protocol could have implemented a logic to allow Makers only settle the total amount of points owed to a Taker.

POC

Changed the pseudo names(users) to real names for more code readability and clarity.

function test_poc() external {
uint userBalBefore = mockUSDCToken.balanceOf((bob));
​
vm.startPrank(bob); // bob wants to sell 1000 points at the rate of 0.01
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
vm.stopPrank();
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
vm.prank(alice); // Alice buys the whole point
preMarktes.createTaker(offerAddr, 1000); // 100 * 0.01 / 1000
​
​
// => Settlements
vm.prank(admin); // owner
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1, // _tge,
3600 // 1 hour // _settlementPeriod
);
​
​
// Bob `Maker`
vm.prank(bob);
deliveryPlace.settleAskMaker(offerAddr, 500);
// Tries to settle the reamining points(500) but fails due the fact that the status has been udated in the previous call
​
vm.prank(bob);
vm.expectRevert();
deliveryPlace.settleAskMaker(offerAddr, 500); //@audit fails
​
​
}

Impact

Could lead to loss of funds for the Takers as they are not able to have their points settled and DOS attack to the protocol.

Mitigation

Restructure the logic either settle the entire points or add a check that only changes the status to OfferStatus.Settled only of all points have been settled.

Updates

Lead Judging Commences

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

finding-DeliveryPlace-settleAskTaker-settleAskMaker-partial-settlements

Valid high, in settleAskTaker/settleAskMaker, if the original offer maker performs a partial final settlement, the existing checks [here](https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L356-L358) and [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L230-L232) will cause an revert when attempting to complete a full settlement, resulting in their collateral being locked and requiring a rescue from the admin. To note, although examples in the documentation implies settlement in a single click, it is not stated that partial settlements are not allowed, so I believe it is a valid user flow.

Support

FAQs

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