Summary
PreMarkets::closeOffer does not refund the full collateral amount to users, specifically failing to return the collateral for points that have been sold. This results in users losing funds that should rightfully be returned to them.
Even though the remaining collateral can be refunded via abortOffer, there are differences in usecases. As per sponsor's comments:
Example:
Maker is selling 100 points
Taker buys 60 points from the offer
If the 2 Takers don't relist the holdings/stock pts, then the Maker can abortOffer and all deals between the Maker and the Takers will be set to INVALID. The Maker get all the collateral fund back. No liability to settle any token.
Whether the Takers relist or not, the Maker can cancelOffer and get the REST collateral (for remaining 40 pts) back. Then no taker can buy pts from this CANCELLED offer. When TGE comes, the Maker still obtain the liability to settle the tokens related to the sold 60 points before.
Therefore, abortAskOffer is used in cases where the offer is completely cancelled and no sales are going to occur, whereas cancelOffer is used in cases where only new offers are cancelled and the old ones stay and should be respected when TGE comes.
That being said, cancelOffer should return any remaining collateral from completed purchases, because they will indeed go through in the future.
Impact
Users who create offers and then close them after partial fills will lose a portion of their collateral. This loss is proportional to the number of points sold before closing the offer.
Proof of Concept
In the PreMarkets::closeOffer function, the refund calculation does not account for the collateral of sold points. OfferLibraries.getRefundAmount only calculates the refund based on the remaining (unsold) points, leaving the collateral for sold points trapped in the contract.
Consider this scenario:
Alice creates an offer for 100,000 points for 10,000 with 110% collateral rate (therefore deposits 11,000).
Bob buys 50,000 points, using 5500 USDC worth of Alice's collateral and paying her 5000 USDC.
Alice decides to close the offer.
The current implementation only refunds Alice's collateral for the remaining 50,000 points (so 5500), effectively losing 500 USDC of her original collateral.
function test_closeOffer_lackingCollateralRefund() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 MAKER_POINTS = 100_000;
uint256 TAKER_POINTS = MAKER_POINTS / 2;
uint256 TOKEN_AMOUNT = 10_000 * 1e18;
uint256 INITIAL_BALANCE = 100_000 * 1e18;
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
console.log("- Alice's balance initially: %18e\n", INITIAL_BALANCE);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
TOKEN_AMOUNT,
10_000 * 1.1,
0,
OfferType.Ask,
OfferSettleType.Protected
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
console.log("> Alice's balance after creating an offer: %18e\n", mockUSDCToken.balanceOf(alice));
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
uint256 balanceAfterPartialFill =
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console.log("> Alice's balance after partial fill: %18e", mockUSDCToken.balanceOf(alice));
console.log("> Alice's userTokenBalance (SalesRevenue) after partial fill: %18e\n", balanceAfterPartialFill);
vm.startPrank(alice);
preMarktes.closeOffer(stockAddr, offerAddr);
uint256 balanceAfterClose =
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
console.log(">> Alice's balance after closing an offer: %18e", mockUSDCToken.balanceOf(alice));
console.log(">> Alice's userTokenBalance (MakerRefund) after closing an offer: %18e\n", balanceAfterClose);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
uint256 CURRENT_BALANCE = mockUSDCToken.balanceOf(alice);
console.log("- Alice's initial balance: %18e", INITIAL_BALANCE);
console.log("- Alice's final balance, after withdrawing sales revenue and refunds: %18e", CURRENT_BALANCE);
assertLt(CURRENT_BALANCE, INITIAL_BALANCE, "Alice's balance is actually larger than it initially was.");
if (INITIAL_BALANCE > CURRENT_BALANCE) {
console.log(unicode"❌ Alice has lost %18e USDC of collateral!", INITIAL_BALANCE - CURRENT_BALANCE);
}
}
Tools Used
Foundry, Manual review
Recommended Mitigation Steps
Modify PreMarkets::closeOffer to refund the full collateral amount, including the portion for sold points:
function closeOffer(address _stock, address _offer) external {
// ... (existing checks)
if (makerInfo.offerSettleType == OfferSettleType.Protected || stockInfo.preOffer == address(0x0)) {
- uint256 refundAmount = OfferLibraries.getRefundAmount(
- offerInfo.offerType, offerInfo.amount, offerInfo.points, offerInfo.usedPoints, offerInfo.collateralRate
- );
+ uint256 totalCollateral = OfferLibraries.getDepositAmount(
+ offerInfo.offerType, offerInfo.collateralRate, offerInfo.amount, true, Math.Rounding.Floor
+ );
+ uint256 usedCollateral = OfferLibraries.getDepositAmount(
+ offerInfo.offerType,
+ offerInfo.collateralRate,
+ offerInfo.amount.mulDiv(offerInfo.usedPoints, offerInfo.points, Math.Rounding.Ceil),
+ false,
+ Math.Rounding.Ceil
+ );
+ uint256 refundAmount = totalCollateral - usedCollateral;
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund, _msgSender(), makerInfo.tokenAddress, refundAmount
);
}
offerInfo.offerStatus = OfferStatus.Canceled;
emit CloseOffer(_offer, _msgSender());
}
