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

Incorrect Token Address Used for Point Token Balance Update in closeBidTaker Function

cryptomoon

Summary

The closeBidTakerin DeliveryPlacecontract is used by takerswho has BIDstock and their's stocks pre-offeris settled. The pre-offerwill be settled in two cases:

1) offeraborted by makerby calling abortAskOffer

2) offersettled by makerby calling settleAskMaker

The closeBidTaker function has a critical issue where the incorrect token address is being used when updating the user's point token balance. Specifically, the function mistakenly uses the makerInfo.tokenAddress, which represents the collateral token, instead of the appropriate point token address when adding the point token balance.

Vulnerability Details

The pointTokenAmount is supposed to represent the amount of point tokens, yet the function uses makerInfo.tokenAddress (which refers to the collateral token) when updating the balance. This is incorrect because point tokens and collateral tokens are different assets, and using the collateral token address in place of the point token address will cause the balance to be added to the wrong token.

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

uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
​
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
userCollateralFee
);
uint256 pointTokenAmount = offerInfo.settledPointTokenAmount.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
_msgSender(),
makerInfo.tokenAddress, // ISSUE: This should be point token and not collateral token
pointTokenAmount
);

Impact

This could result in users receiving the wrong tokens. If collateral token is worth very much than point token, users would get collateral token instead of point tokens causing loss to the protocol.

Tools Used

Manual review

Recommendations

tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
_msgSender(),
marketplaceInfo.tokenAddress, // FIX: This should be point token
pointTokenAmount
);
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!