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

Partially settling ask maker would not refund maker collateral tokens

baz1ka

Vulnerability Details

DeliveryPlace.settleAskMaker() allows Ask offer owner to send point tokens to protocol to settle offer and get back previously provided collateral tokens. This function has condition that checks whether provided amount of point tokens is the same as usedPoints of offer:

);
}
​
uint256 makerRefundAmount;
@> if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,

If user provides the same amount of point tokens as usedAmount then owner gets back collateral tokens., but if user provides less he becomes ineligible for getting back collateral tokens. For various reasons user can not provide promised amount of point tokens and would hope to get back as many collateral tokens as he provides.

Impact

This can happen if maker for some reason can not provide enough point tokens. For example:

  • protocol didn't give user enough point tokens

  • maker wanted to sell point tokens by having multiple account eligible in airdrop but some accounts (even 1) didn't get airdrop

  • user lost some point tokens in other unforeseen events (accidentally sending even 1 wei of point token would be enough to not get collateral tokens back)

  • protocol has a vesting and user can immediately withdraw only part of point tokens

Proof of Concept

Modify test/PreMarkets.t.sol test with the next code and execute this test with forge test --match-test test_settle_ask_maker_maker_lose_funds:

function test_settle_ask_maker_maker_lose_funds() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
preMarktes.relistOffer(stock1Addr, offer1Addr);
​
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
uint256 beforeSettleMakerSalesRevenue = tokenManager
.userTokenBalanceMap(
user,
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
​
// Offer owner don't have enough point tokens but still try to settle offer as want to get back part of collateral tokens
deliveryPlace.settleAskMaker(offerAddr, 100);
vm.stopPrank();
​
uint256 refundedTokensAmount = tokenManager.userTokenBalanceMap(
user,
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
uint256 afterSettleMakerSalesRevenue = tokenManager.userTokenBalanceMap(
user,
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
// SalesRevenue didn't change because user is not refunded for settledTokens that is less than usedTokens
assertEq(beforeSettleMakerSalesRevenue, afterSettleMakerSalesRevenue);
}

Recommendations

Allow offer owner to get refunded even for part of point tokens.

Updates

Lead Judging Commences

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

Give us feedback!