Submission Details
Severity:
Incorrect authorization check in `DeliveryPlace::settleAskTaker` allows maker to settle taker's ask order and mistakenly trigger a deposit instead of the taker
Summary
DeliveryPlace::settleAskTaker has an incorrect authorization check that allows the maker to settle the taker's ask instead of the taker themselves. It also incorrectly requires makers to deposit tokens to settle takers' orders.
// src/core/DeliveryPlace.sol
function settleAskTaker(address _stock, uint256 _settledPoints) external {
// previous code...
if (status == MarketPlaceStatus.AskSettling) {
@> if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
}else {
// existing code...
}
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint * _settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
@> tokenManager.tillIn(_msgSender(), marketPlaceInfo.tokenAddress, settledPointTokenAmount, true);
// the rest of the function...
}
}
Impact
This vulnerability can lead to unintended financial losses for makers as they are forced to deposit their own tokens to settle orders that should be settled by takers.
Proof of Concept
Place the following test in PreMarkets.t.sol:
function test_makerDepositsInsteadOfTaker_issue() public {
// ~~~~~~~~~~~~~~~~~~~~ Setup ~~~~~~~~~~~~~~~~~~~~
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 INITIAL_BALANCE = 1000 * 1e18;
uint256 MAKER_POINTS = 10_000;
uint256 TAKER_POINTS = 5_000;
// Provide Alice and Bob with initial token balances
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 1) Create a maker offer ~~~~~~~~~~
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
0.01 * 1e18,
10_000 * 1.2, // 120% collateral rate
10_000 * 0.05, // 5% each trade tax
OfferType.Bid,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 2) Create a taker order ~~~~~~~~~~
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
address stockAddr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 3) Update marketplace to AskSettling status ~~~~~~~~~~
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockUSDCToken), 0.01 * 1e18, block.timestamp - 1, 3600);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// NOTE: with current implementation, if Bob tries to trigger the function, he'll run into `revert Errors.Unauthorized()`
// therefore, we're running the test in order to allow it to pass through the authorization and demonstrate the currently available flow
// by calling the function as Alice, who's a maker in this case
// ~~~~~~~~~~ 4) Issue: Alice attempts to settle the ask, which should be done by Bob ~~~~~~~~~~
vm.startPrank(alice);
uint256 aliceBalanceBeforeSettlement = mockUSDCToken.balanceOf(alice);
uint256 bobBalanceBeforeSettlement = mockUSDCToken.balanceOf(bob);
deliveryPlace.settleAskTaker(stockAddr, TAKER_POINTS); // This should not change Alice's balance if correct
uint256 aliceBalanceAfterSettlement = mockUSDCToken.balanceOf(alice);
uint256 bobBalanceAfterSettlement = mockUSDCToken.balanceOf(bob);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
console.log("> Alice's balance BEFORE settleAskTaker: %18e", aliceBalanceBeforeSettlement);
console.log("> Bob's balance BEFORE settleAskTaker: %18e\n", bobBalanceBeforeSettlement);
console.log(">> Alice's balance AFTER settleAskTaker: %18e", aliceBalanceAfterSettlement);
console.log(">> Bob's balance AFTER settleAskTaker: %18e\n", bobBalanceAfterSettlement);
// Check balances to confirm if Alice's balance decreased, indicating she deposited tokens
assertEq(
aliceBalanceBeforeSettlement,
aliceBalanceAfterSettlement,
"Alice's balance should not change, indicating incorrect token flow."
);
}
The aforementioned test should yield the following output:
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[FAIL. Reason: assertion failed] test_makerDepositsInsteadOfTaker_issue() (gas: 1293294)
Logs:
> Alice's balance BEFORE settleAskTaker: 999.99
> Bob's balance BEFORE settleAskTaker: 999.993725
>> Alice's balance AFTER settleAskTaker: 949.99
>> Bob's balance AFTER settleAskTaker: 999.993725
Error: Alice's balance should not change, indicating incorrect token flow.
Error: a == b not satisfied [uint]
Expected: 949990000000000000000
Actual: 999990000000000000000
Recommended Mitigation Steps
Correct the authorization check to ensure that only the taker can call settleAskTaker:
function settleAskTaker(address _stock, uint256 _settledPoints) external {
// previous code...
if (status == MarketPlaceStatus.AskSettling) {
- if (_msgSender() != offerInfo.authority) {
+ if (_msgSender() != stockInfo.authority) {
revert Errors.Unauthorized();
}
}else{
// the rest of the function...
}
}
Now we can update the previous test to allow Bob to settle his order:
function test_makerDepositsInsteadOfTaker_fixed() public {
// ~~~~~~~~~~~~~~~~~~~~ Setup ~~~~~~~~~~~~~~~~~~~~
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 INITIAL_BALANCE = 1000 * 1e18;
uint256 MAKER_POINTS = 10_000;
uint256 TAKER_POINTS = 5_000;
// Provide Alice and Bob with initial token balances
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 1) Create a maker offer ~~~~~~~~~~
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
0.01 * 1e18,
10_000 * 1.2, // 120% collateral rate
10_000 * 0.05, // 5% each trade tax
OfferType.Bid,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 2) Create a taker order ~~~~~~~~~~
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
address stockAddr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 3) Update marketplace to AskSettling status ~~~~~~~~~~
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockUSDCToken), 0.01 * 1e18, block.timestamp - 1, 3600);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// ~~~~~~~~~~ 4) Fix: Now Bob attempts to settle the ask, not Alice ~~~~~~~~~~ <<<<
vm.startPrank(bob);
uint256 aliceBalanceBeforeSettlement = mockUSDCToken.balanceOf(alice);
uint256 bobBalanceBeforeSettlement = mockUSDCToken.balanceOf(bob);
deliveryPlace.settleAskTaker(stockAddr, TAKER_POINTS); // This should not change Alice's balance if correct
uint256 aliceBalanceAfterSettlement = mockUSDCToken.balanceOf(alice);
uint256 bobBalanceAfterSettlement = mockUSDCToken.balanceOf(bob);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
console.log("> Alice's balance BEFORE settleAskTaker: %18e", aliceBalanceBeforeSettlement);
console.log("> Bob's balance BEFORE settleAskTaker: %18e\n", bobBalanceBeforeSettlement);
console.log(">> Alice's balance AFTER settleAskTaker: %18e", aliceBalanceAfterSettlement);
console.log(">> Bob's balance AFTER settleAskTaker: %18e\n", bobBalanceAfterSettlement);
// Check balances to confirm if Alice's balance decreased, indicating she deposited tokens
assertEq(
aliceBalanceBeforeSettlement,
aliceBalanceAfterSettlement,
"Alice's balance should not change, indicating incorrect token flow."
);
}
The updated test should yield the following output:
[PASS] test_makerDepositsInsteadOfTaker_fixed() (gas: 1278601)
Logs:
> Alice's balance BEFORE settleAskTaker: 999.99
> Bob's balance BEFORE settleAskTaker: 999.993725
>> Alice's balance AFTER settleAskTaker: 999.99
>> Bob's balance AFTER settleAskTaker: 949.993725
Updates
Lead Judging Commences
0xnevi over 1 year ago
Submission Judgement Published Assigned finding tags:
finding-PreMarkets-settleAskTaker-wrong-stock-authority
Valid high severity, when taker offers are created pointing to a `offer`, the relevant `stockInfoMap` offers are created with the owner of the offer aka `authority`, set as the creater of the offer, as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L245). Because of the wrong check within settleAskTaker, it will permanently DoS the final settlement functionality for taker offers for the maker that listed the original offer, essentially bricking the whole functionality of the market i.e. maker will always get refunded the original collateral, and takers will never be able to transact the original points put up by the maker. This occurs regardless of market mode.
Prize pool breakdown
Total prize
27,750 USDC
nSLOC
1,22923
USDC / LOC
25,000 USDC
2,750 USDC
2,250 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.