Summary
DeliveryPlace::settleAskTaker incorrectly requires the taker to deposit tokens again during settlement, even though they have already provided tokens when creating the taker order. This leads to a double deposit situation, potentially causing the taker to lose funds.
DeliveryPlace.sol#L377-L382
function settleAskTaker(address _stock, uint256 _settledPoints) external {
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint * _settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
@> tokenManager.tillIn(_msgSender(), marketPlaceInfo.tokenAddress, settledPointTokenAmount, true);
tokenManager.addTokenBalance(
TokenBalanceType.PointToken, offerInfo.authority, makerInfo.tokenAddress, settledPointTokenAmount
);
}
}
Impact
When settling an ask order, takers are forced to deposit tokens they've already provided, effectively doubling their required contribution. This not only locks up more of the taker's funds than necessary but also creates an imbalance in the protocol's token accounting.
In extreme cases, if a taker doesn't have sufficient additional tokens, they might be unable to settle their own orders, leading to failed transactions and potential loss of opportunities.
Proof of Concept
NOTE: for this test to run, my other finding by the title of "Incorrect authorization check in DeliveryPlace::settleAskTaker allows maker to settle taker's ask order" must be resolved first. It will fix the authorization issue of the function and allow us to continue with the flow of taker settling their offer.
Place the following test in PreMarkets.t.sol:
function test_settleAskTaker_takerDoubleDeposit() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 INITIAL_BALANCE = 1000 * 1e18;
uint256 MAKER_POINTS = 10_000;
uint256 TAKER_POINTS = 5_000;
uint256 TOKEN_AMOUNT = 100 * 1e18;
uint256 COLLATERAL_RATE = 10_000 * 1.2;
uint256 EACH_TRADE_TAX = 10_000 * 0.03;
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
TOKEN_AMOUNT,
COLLATERAL_RATE,
EACH_TRADE_TAX,
OfferType.Bid,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
console.log("> Bob's actual balance BEFORE createTaker: %18e", mockUSDCToken.balanceOf(bob));
preMarktes.createTaker(offerAddr, TAKER_POINTS);
console.log(
"> Bob's actual balance AFTER createTaker (and after deposit): %18e\n", mockUSDCToken.balanceOf(bob)
);
address stockAddr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockUSDCToken), 0.01 * 1e18, block.timestamp - 1, 3600);
vm.stopPrank();
vm.startPrank(bob);
uint256 bobBalanceBeforeSettlement = mockUSDCToken.balanceOf(bob);
deliveryPlace.settleAskTaker(stockAddr, TAKER_POINTS);
uint256 bobBalanceAfterSettlement = mockUSDCToken.balanceOf(bob);
console.log(">> Bob's actual balance BEFORE settleAskTaker: %18e", bobBalanceBeforeSettlement);
console.log(
">> Bob's actual balance AFTER settleAskTaker (and after second deposit): %18e\n", bobBalanceAfterSettlement
);
uint256 bobSalesRevenue =
tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
uint256 collateralRefund =
tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.RemainingCash);
console.log("- Bob's sales revenue: %18e", bobSalesRevenue);
console.log("- Bob's collateral refund: %18e", collateralRefund);
uint256 expectedFinalBalance = bobBalanceBeforeSettlement + bobSalesRevenue + collateralRefund;
uint256 actualFinalBalance = bobBalanceAfterSettlement + bobSalesRevenue + collateralRefund;
console.log(">> Bob's expected final balance: %18e", expectedFinalBalance);
console.log(">> Bob's actual final balance: %18e\n", actualFinalBalance);
assertLt(
actualFinalBalance,
expectedFinalBalance,
"Bob's final balance should be higher than initial, accounting for token sale"
);
console.log(
">>>> Bob's funds are reduced due to double deposit. Lost amount: %18e",
expectedFinalBalance - actualFinalBalance
);
}
Running forge test --mt test_settleAskTaker_takerDoubleDeposit -vvv should yield the following output:
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_settleAskTaker_takerDoubleDeposit() (gas: 1291577)
Logs:
> Bob's actual balance BEFORE createTaker: 1000
> Bob's actual balance AFTER createTaker (and after deposit): 938.25
>> Bob's actual balance BEFORE settleAskTaker: 938.25
>> Bob's actual balance AFTER settleAskTaker (and after second deposit): 888.25
- Bob's sales revenue: 50
- Bob's collateral refund: 60
>> Bob's expected final balance: 1048.25
>> Bob's actual final balance: 998.25
>>>> Bob's funds are reduced due to double deposit. Lost amount: 50
This test shows that due to double deposit (one in createTaker and another one in settleAskTaker), Bob lost 50 tokens, which is exactly the amount of the second deposit.
Tools Used
Foundry, Manual code review
Recommended Mitigation Steps
Remove the additional token deposit requirement in the settleAskTaker function.
Ensure that the function only handles the settlement process by transferring the already deposited tokens from the contract to the maker, by increasing internal balance tracking:
function settleAskTaker(address _stock, uint256 _settledPoints) external {
// existing code...
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint * _settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
- tokenManager.tillIn(_msgSender(), marketPlaceInfo.tokenAddress, settledPointTokenAmount, true);
tokenManager.addTokenBalance(
TokenBalanceType.PointToken, offerInfo.authority, makerInfo.tokenAddress, settledPointTokenAmount
);
}
// the rest of the function...
}
