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

Collateral fees are stuck when Admin settles ASK Offer instead of Maker

vinica_boy

Summary

When a Maker fails to settle an ASK Offer, the expectation is that the Admin will settle on their behalf, ensuring that the BID Taker receives a portion of the collateral, with the remaining portion allocated to the protocol team. This process relies on accurately accounting for the tokens held by different addresses. However, in the current implementation, when the Admin settles instead of the Maker, the collateral fee is not correctly attributed to the Admin. This results in the funds becoming stuck, as there would be no mechanism for the Admin to withdraw these funds.

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L257
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L276
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L301

Admin is expected to call settleAskMaker() in DeliveryPlace.sol instead of Maker.
On line #257 is checked if input parameter _settledPoints is exactly 0 (this is the only value which satisfies the _settledPoints <= 0 condition, because _settledPoints is of unsigned type).
Later, on line #276 there is another check _settledPoints == offerInfo.usedPoints which will be true only if offerInfo.usedPoints is 0, which is not the case when there are BID Takers to the ASK Offer.
This leads to skipping the code from line #301-#306 which account for the allowance of token withdrawal for the current msg.sender (in this scenario - Admin).
Because of this inconsistency, Admin wont be able to call withdraw() in TokenManager.sol later as his accounted allowance would be 0, instead of the remaining collateral.

The following test case shows the described scenario.
NOTE: need to add deal(user2, 100000000 * 10 ** 18); in setUp().

function testAdminSettleIsNotAccounted() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace, address(weth9), 1000, 0.01 * 1e18, 12000, 300, OfferType.Ask, OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
vm.prank(user2);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
​
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
// Admin settles instead of Maker after settlement period is finished
// warp to set timestamp after settlement period is finished
vm.warp(1719826111275);
deliveryPlace.settleAskMaker(offerAddr, 0);
vm.stopPrank();
​
vm.startPrank(user2);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.closeBidTaker(stock1Addr);
​
// Taker gets his part of the collateral correctly.
console.log(tokenManager.userTokenBalanceMap(address(user2), address(address(weth9)), TokenBalanceType.RemainingCash));
// Admin is not accounted correctly and balances remain 0.
console.log(tokenManager.userTokenBalanceMap(address(user1), address(weth9), TokenBalanceType.MakerRefund));
console.log(tokenManager.userTokenBalanceMap(address(user1), address(weth9), TokenBalanceType.SalesRevenue));
vm.stopPrank();
}

Impact

Collateral of Maker who do not settle remains stuck in the protocol due to wrong accounting.

Tools Used

Manual review, Foundry.

Recommendations

Consider accounting for the collateral fee, when settling ASK Offers as Admin instead of the Maker who created the Offer.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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