Summary
In the PreMarkets::createOffer() function, the offerId stored in the offerInfoMap and stockInfoMap does not match the actually generated offerAddr and stockAddr.
Vulnerability Details
In the PreMarkets::createOffer() function, offerId is incremented after generating makerAddr, offerAddr, and stockAddr, which causes the offerId stored in offerInfoMap and stockInfoMap to not match the actually generated offerAddr and stockAddr. Since offerId is incremented after generating these addresses, the offerId when updating the storage is 1 greater than the offerId used for the actual generated address.
function createOffer(CreateOfferParams calldata params) external payable {
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
@> address offerAddr = GenerateAddress.generateOfferAddress(offerId);
@> address stockAddr = GenerateAddress.generateStockAddress(offerId);
@> offerId = offerId + 1;
offerInfoMap[offerAddr] = OfferInfo({
@> id: offerId,
authority: _msgSender(),
maker: makerAddr,
offerStatus: OfferStatus.Virgin,
offerType: params.offerType,
points: params.points,
amount: params.amount,
collateralRate: params.collateralRate,
abortOfferStatus: AbortOfferStatus.Initialized,
usedPoints: 0,
tradeTax: 0,
settledPoints: 0,
settledPointTokenAmount: 0,
settledCollateralAmount: 0
});
stockInfoMap[stockAddr] = StockInfo({
@> id: offerId,
stockStatus: StockStatus.Initialized,
stockType: params.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: makerAddr,
preOffer: address(0x0),
offer: offerAddr,
points: params.points,
amount: params.amount
});
emit CreateOffer(
offerAddr,
makerAddr,
stockAddr,
params.marketPlace,
_msgSender(),
params.points,
params.amount
);
}
}
And id is not the only mark in the comment
* @title StockInfo
* @dev Struct of StockInfo
* @notice id, stockStatus, stockType, authority, maker, preOffer, points, amount, offer
@> * @param id the unique id of stock.
* @param stockStatus the status of stock, detail in OfferStatus.sol.
* @param stockType the type of stock, detail in OfferStatus.sol.
* @param authority the owner of stock.
* @param maker the maker of stock, is a virtual address, storage as MakerInfo.
* @param preOffer the preOffer of stock.
* @param points the points of sell or buy stock.
* @param amount receive or used collateral amount when sell or buy.
* @param offer the offer of stock, is a virtual address, storage as OfferInfo.
*/
struct StockInfo {
uint256 id;
StockStatus stockStatus;
StockType stockType;
address authority;
address maker;
address preOffer;
uint256 points;
uint256 amount;
address offer;
}
Poc
Please add the test code to test/PreMarkets.t.sol and execute
function testID_is_not_unique() public {
vm.startPrank(user);
assert(preMarktes.offerId() == 0);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address userStockAddr = GenerateAddress.generateStockAddress(0);
address userOfferAddr = GenerateAddress.generateOfferAddress(0);
assert(preMarktes.offerId() == 1);
preMarktes.createTaker(userOfferAddr, 500);
address userCreateTakerStockAddr = GenerateAddress.generateStockAddress(1);
assert(userCreateTakerStockAddr != userStockAddr);
assertEq(preMarktes.getStockInfo(userStockAddr).id,preMarktes.getStockInfo(userCreateTakerStockAddr).id);
console2.log("preMarktes.getStockInfo(userStockAddr).id:",preMarktes.getStockInfo(userStockAddr).id);
console2.log("preMarktes.getStockInfo(userCreateTakerStockAddr).id:",preMarktes.getStockInfo(userCreateTakerStockAddr).id);
vm.stopPrank();
}
Code Snippet
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L39-L157
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L295-L396
Impact
In the PreMarkets::createOffer() function, the offerId stored in the offerInfoMap and stockInfoMap does not match the actually generated offerAddr and stockAddr.
Tools Used
Manual Review
Recommendations
Consider moving the increment of offerId to the end of the function, or cache offerId in currentOfferId for use.
function createOffer(CreateOfferParams calldata params) external payable {
// SNIP..
/// @dev generate address for maker, offer, stock.
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
// SNIP..
- offerId = offerId + 1;
// SNIP...
/// @dev update offer info
offerInfoMap[offerAddr] = OfferInfo({
id: offerId,
authority: _msgSender(),
maker: makerAddr,
offerStatus: OfferStatus.Virgin,
offerType: params.offerType,
points: params.points,
amount: params.amount,
collateralRate: params.collateralRate,
abortOfferStatus: AbortOfferStatus.Initialized,
usedPoints: 0,
tradeTax: 0,
settledPoints: 0,
settledPointTokenAmount: 0,
settledCollateralAmount: 0
});
/// @dev update stock info
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
stockType: params.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: makerAddr,
preOffer: address(0x0),
offer: offerAddr,
points: params.points,
amount: params.amount
});
+ offerId = offerId + 1;
emit CreateOffer(
offerAddr,
makerAddr,
stockAddr,
params.marketPlace,
_msgSender(),
params.points,
params.amount
);
}
}
