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

Wrong accounting of points token for BID Makers can lead to users withdrawing more collateral than what they have deposited.

vinica_boy

Summary

When a user wants to purchase points in a specific marketplace at a price they are willing to pay, they can create a BID Offer. Conversely, users who are ready to sell their points at that price can place an ASK Order. Once the Token Generation Event (TGE) occurs, sellers are responsible for fulfilling the deal by transferring the points to the buyer. Sellers are incentivized to complete the transaction because they have posted collateral when they created the ASK Order, ensuring their commitment.

In code terms, Takers must transfer the points tokens via the settleAskTaker() function in DeliveryPlace.sol. The transfer process isn't direct—Takers transfer the tokens to the protocol, and the virtual balance of the Maker is updated in TokenManager.sol. However, there is an issue: the settleAskTaker() function currently does not account correctly when Takers settle, leading to potential discrepancies in the Maker's balance and affecting the overall transaction integrity.

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L384
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L109

On line #384 in DeliveryPlace.sol the wrong token address is updated. As we are updating the balance for the PointToken we must provide the corresponding address.

tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
offerInfo.authority,
makerInfo.tokenAddress,
settledPointTokenAmount
);

makerInfo.tokenAddress is the address of the collateral token which is set initially when Offer is created by a Maker in createOffer() in PreMarkets.sol on line #109.

Using the protocol test file, I have added another test case which shows the described issue.
NOTE: need to add the following line: import {MarketPlaceStatus} from "../src/interfaces/ISystemConfig.sol";

function testSettleAskTakerPoC() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
vm.prank(user2);
preMarktes.createTaker(offerAddr, 500);
​
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
systemConfig.updateMarketPlaceStatus("Backpack", MarketPlaceStatus.AskSettling);
vm.startPrank(user2);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
// Next call would revert with Errors.Unauthorized()
deliveryPlace.settleAskTaker(stock1Addr, 500);
vm.stopPrank();
// log accounted value = 0
console.log(tokenManager.userTokenBalanceMap(user, address(mockPointToken), TokenBalanceType.PointToken));
vm.prank(user);
tokenManager.withdraw(address(mockPointToken), TokenBalanceType.PointToken);
// Fails as user balance stays the same is initial
assertGt(mockPointToken.balanceOf(user), 100000000 * 10 ** 18);
}

Impact

The incorrect accounting in the settleAskTaker() function poses a significant risk to the protocol. Users could potentially exploit this vulnerability to withdraw collateral tokens instead of the intended point tokens. This could lead to severe financial losses for the protocol, as collateral meant to secure transactions could be drained improperly. Moreover, the integrity of the marketplace would be compromised.

Tools Used

Manual review, Foundry.

Recommendations

Instead of makerInfo.tokenAddress which is the address of the collateral token, user marketPlaceInfo.tokenAddress which is the address of the point token.

Updates

Lead Judging Commences

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

finding-DeliveryPlace-settleAskTaker-closeBidTaker-wrong-makerinfo-token-address-addToken-balance

Valid high severity, In `settleAskTaker/closeBidTaker`, by assigning collateral token to user balance instead of point token, if collateral token is worth more than point, this can cause stealing of other users collateral tokens within the CapitalPool contract, If the opposite occurs, user loses funds based on the points they are supposed to receive

Support

FAQs

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

Give us feedback!