Summary
The addTokenBalance function within the TokenManager.sol contract does not include a check to prevent zero-value _amount parameters. This allows the function to emit events and modify state with potentially unintended zero-value transactions.
Vulnerability Details
This is the addTokenBalance function code block.
function addTokenBalance(
TokenBalanceType _tokenBalanceType,
address _accountAddress,
address _tokenAddress,
uint256 _amount
) external onlyRelatedContracts(tadleFactory, _msgSender()) {
userTokenBalanceMap[_accountAddress][_tokenAddress][
_tokenBalanceType
] += _amount;
emit AddTokenBalance(
_accountAddress,
_tokenAddress,
_tokenBalanceType,
_amount
);
}
As you can see, this function lacks validation to ensure that the _amount parameter is greater than zero.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L113-L129
Impact
** **Emitting events with a zero-value _amount can clutter logs with unnecessary data, making it difficult to analyze meaningful transactions. And even though adding zero to a balance does not alter the numeric value, it still invokes state change operations that are unnecessary and could increment gas costs.
Proof of Concept
This is the test code.
function test_ask_offer_turbo_usdc() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
preMarktes.relistOffer(stock1Addr, offer1Addr);
vm.stopPrank();
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(offerAddr, 500);
deliveryPlace.closeBidTaker(stock1Addr);
vm.stopPrank();
}
The result is like this.
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 3, amount: 0)
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ [28076] UpgradeableProxy::addTokenBalance(5, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 5000000000000000000 [5e18])
│ │ │ ├─ [27549] TokenManager::addTokenBalance(5, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 5000000000000000000 [5e18]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
As you can see, an event with zero amount was emitted.
Tools Used
Manual code review
Recommendations
Add a require statement at the beginning of the addTokenBalance function to check that the _amount parameter is greater than zero.
function addTokenBalance(
TokenBalanceType _tokenBalanceType,
address _accountAddress,
address _tokenAddress,
uint256 _amount
) external onlyRelatedContracts(tadleFactory, _msgSender()) {
require(_amount > 0, "Amount must be greater than zero");
userTokenBalanceMap[_accountAddress][_tokenAddress][
_tokenBalanceType
] += _amount;
emit AddTokenBalance(
_accountAddress,
_tokenAddress,
_tokenBalanceType,
_amount
);
}
