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

Bid taker gets no refund when they abort

z3r0

Summary

When an bid taker creates a taker using PreMarkets.sol::createTaker(), the takers deposit amount is not added to TokenBalanceType.MakerRefund refund balances when they abort their bid - due to the following miscalculation.

Vulnerability Details

An offer maker has the option to abort their ask offer using PreMarkets.sol::abortAskOffer(). When this happens this original ask offer's offerInfo.abortOfferStatus is changed to AbortOfferStatus.Aborted:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L631

offerInfo.abortOfferStatus = AbortOfferStatus.Aborted;

After this, a bid taker can now abort their bid on that aborted ask offer using PreMarkets.sol::abortBidTaker() - this is the normal contract flow. During this call, the bid taker's refund balance is meant to be calculated and saved, but the calculation order is messed up:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L671-L675

uint256 depositAmount = stockInfo.points.mulDiv( // @audit: wrong alignment, should be points.mulDiv(preOfferInfo.amount, preOfferInfo.points)
preOfferInfo.points,
preOfferInfo.amount,
Math.Rounding.Floor
);

This resolves to 0, as in the test suite points are around 1000. So if the bid taker initially creates a bid of 500 points, the calculation resolves to 0:

offer's point amount = 1000
user's point bid = 500
offer's amount = 1e18 (1 ETH)
​
deposit_amount = 500 * 1000 / 1e18
= 500000 / 1e18 // @audit (resolves to 0 in the EVM)

Thus, the user gets no refunds due to the wrong order of calculation.

For further investigation, even running the test case test_abort_turbo_offer() reveals this bug. Let's examine the stack trace:

... omitted early traces for brevity
...
...
│ │ ├─ emit CreateTaker(offer: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, stock: 0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, points: 500, amount: 5000000000000000 [5e15], tradeTax: 150000000000000 [1.5e14], platformFee: 25000000000000 [2.5e13])
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("user1 balance after creating taker with 500 points: %d", 5) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← [Return]
├─ [41602] UpgradeableProxy::abortAskOffer(0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F, 0xE619a2899a8db14983538159ccE0d238074a235d)
│ ├─ [41087] PreMarktes::abortAskOffer(0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F, 0xE619a2899a8db14983538159ccE0d238074a235d) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(1) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0xffD4505B3452Dc22f8473616d50503bA9E1710Ac]
│ │ ├─ [2303] UpgradeableProxy::getMarketPlaceInfo(0xE6b1c25C9BAC2B628d6E2d231F9B53b92172fC2D) [staticcall]
│ │ │ ├─ [1764] SystemConfig::getMarketPlaceInfo(0xE6b1c25C9BAC2B628d6E2d231F9B53b92172fC2D) [delegatecall]
│ │ │ │ └─ ← [Return] MarketPlaceInfo({ fixedratio: false, status: 1, tokenAddress: 0x0000000000000000000000000000000000000000, tokenPerPoint: 0, tge: 0, settlementPeriod: 0 })
│ │ │ └─ ← [Return] MarketPlaceInfo({ fixedratio: false, status: 1, tokenAddress: 0x0000000000000000000000000000000000000000, tokenPerPoint: 0, tge: 0, settlementPeriod: 0 })
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ ├─ [28026] UpgradeableProxy::addTokenBalance(4, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 7000000000000000 [7e15])
│ │ │ ├─ [27499] TokenManager::addTokenBalance(4, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 7000000000000000 [7e15]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 7000000000000000 [7e15])
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ emit AbortAskOffer(offer: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::startPrank(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF)
│ └─ ← [Return]
├─ [15108] UpgradeableProxy::abortBidTaker(0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, 0xE619a2899a8db14983538159ccE0d238074a235d)
│ ├─ [14593] PreMarktes::abortBidTaker(0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, 0xE619a2899a8db14983538159ccE0d238074a235d) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ ├─ [8126] UpgradeableProxy::addTokenBalance(4, 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 0)
│ │ │ ├─ [7599] TokenManager::addTokenBalance(4, 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 0) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 0)
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ emit AbortBidTaker(stock: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]

Looking at the CreateTaker event emitted in the log, you can see that the amount supplied by the user is amount: 5000000000000000 [5e15]. When abortBidTaker() is called, a AddTokenBalance event is emitted:

│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 0)

The amount emitted is set to 0 as we can see, mind you - the account being called is the refund address TokenBalanceType.MakerRefund. This sets the users refund balance to 0.

Impact

User loses all funds in the event of an aborted ask offer.

Tools Used

Manual && Foundry

Recommendations

Simply swapping the order of the operation is enough:

uint256 depositAmount = stockInfo.points.mulDiv( // @audit_fix: swapped `preOrderInfo.amount` and `preOrderInfo.points`
preOfferInfo.amount,
preOfferInfo.points,
Math.Rounding.Floor
);

This fixes the error, as if we run the same test case - we can see by the events emitted, that the user now gets the appropriate refund:

│ │ ├─ emit CreateTaker(offer: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, stock: 0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, points: 500, amount: 5000000000000000 [5e15], tradeTax: 150000000000000 [1.5e14], platformFee: 25000000000000 [2.5e13])
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("user1 balance after creating taker with 500 points: %d", 5) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← [Return]
├─ [41602] UpgradeableProxy::abortAskOffer(0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F, 0xE619a2899a8db14983538159ccE0d238074a235d)
│ ├─ [41087] PreMarktes::abortAskOffer(0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F, 0xE619a2899a8db14983538159ccE0d238074a235d) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(1) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0xffD4505B3452Dc22f8473616d50503bA9E1710Ac]
│ │ ├─ [2303] UpgradeableProxy::getMarketPlaceInfo(0xE6b1c25C9BAC2B628d6E2d231F9B53b92172fC2D) [staticcall]
│ │ │ ├─ [1764] SystemConfig::getMarketPlaceInfo(0xE6b1c25C9BAC2B628d6E2d231F9B53b92172fC2D) [delegatecall]
│ │ │ │ └─ ← [Return] MarketPlaceInfo({ fixedratio: false, status: 1, tokenAddress: 0x0000000000000000000000000000000000000000, tokenPerPoint: 0, tge: 0, settlementPeriod: 0 })
│ │ │ └─ ← [Return] MarketPlaceInfo({ fixedratio: false, status: 1, tokenAddress: 0x0000000000000000000000000000000000000000, tokenPerPoint: 0, tge: 0, settlementPeriod: 0 })
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ ├─ [28026] UpgradeableProxy::addTokenBalance(4, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 7000000000000000 [7e15])
│ │ │ ├─ [27499] TokenManager::addTokenBalance(4, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 7000000000000000 [7e15]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 7000000000000000 [7e15])
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ emit AbortAskOffer(offer: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::startPrank(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF)
│ └─ ← [Return]
├─ [35011] UpgradeableProxy::abortBidTaker(0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, 0xE619a2899a8db14983538159ccE0d238074a235d)
│ ├─ [34496] PreMarktes::abortBidTaker(0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, 0xE619a2899a8db14983538159ccE0d238074a235d) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ ├─ [28026] UpgradeableProxy::addTokenBalance(4, 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 5000000000000000 [5e15])
│ │ │ ├─ [27499] TokenManager::addTokenBalance(4, 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 5000000000000000 [5e15]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 5000000000000000 [5e15])
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ emit AbortBidTaker(stock: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-abortBidTaker-amount-wrong-StockInfo-points

Valid high severity, due to incorrect computation of `depositAmount` within `abortBidTaker`, when aborting bid offers created by takers, the collateral refund will be completely wrong for the taker, and depending on the difference between the value of `points` and `amount`, it can possibly even round down to zero, causing definite loss of funds. If not, if points were worth less than the collateral, this could instead be used to drain the CapitalPool contract instead.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!