Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Arithmetic over/underflow

moh469

Summary : The withdraw function in TradingAccount.sol encounters an arithmetic over/underflow error when handling negative values due to the sub function. Although withdraw is an internal function and the functions that call it handle these scenarios appropriately, it would be beneficial to manage this at the sub function level to prevent potential issues. While this change might increase gas costs, it would improve overall robustness. This adjustment also addresses another related issue I raised in a separate submission.

function withdraw(Data storage self, address collateralType, UD60x18 amountX18) internal {
// load address : collateral map for this account
EnumerableMap.AddressToUintMap storage marginCollateralBalanceX18 = self.marginCollateralBalanceX18;
​
// get currently deposited scaled-to-18-decimals this account has
// for this collateral type, then subtracts newly withdrawn amount also scaled to 18 decimals
UD60x18 newMarginCollateralBalance = getMarginCollateralBalance(self, collateralType).sub(amountX18);
​
if (newMarginCollateralBalance.isZero()) {
// if no more collateral, remove this collateral type
// from this account's deposited collateral
marginCollateralBalanceX18.remove(collateralType);
} else {
// else convert updated scaled-to-18-decimals UD60x18 to uint256 and stores
// it in address : collateral map for this account
marginCollateralBalanceX18.set(collateralType, newMarginCollateralBalance.intoUint256());
}
​
// load collateral configuration for this collateral type
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// update total deposited for this collateral type
marginCollateralConfiguration.totalDeposited =
ud60x18(marginCollateralConfiguration.totalDeposited).sub(amountX18).intoUint256();
}

Vulnerability Details:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;
​
// Zaros dependencies
import { Base_Test } from "test/Base.t.sol";
import "forge-std/Test.sol";
​
// PRB Math dependencies
import { UD60x18 } from "@prb-math/UD60x18.sol";
​
contract Withdraw_Unit_Test is Base_Test {
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarkets();
changePrank({ msgSender: users.naruto.account });
}
​
function testFuzz_AttemptWithdrawMoreThanBalance(uint256 amountToDeposit) external {
amountToDeposit = bound({
x: amountToDeposit,
min: WSTETH_MIN_DEPOSIT_MARGIN,
max: convertUd60x18ToTokenAmount(address(wstEth), WSTETH_DEPOSIT_CAP_X18)
});
​
deal({ token: address(wstEth), to: users.naruto.account, give: amountToDeposit });
​
uint128 tradingAccountId = createAccountAndDeposit(amountToDeposit, address(wstEth));
​
UD60x18 excessWithdrawAmountX18 = convertTokenAmountToUd60x18(address(wstEth), amountToDeposit + 2);
console.log("NEW YEAR");
console.log("trading account: %d", tradingAccountId);
​
// vm.expectRevert("Insufficient balance");
perpsEngine.exposed_withdraw(tradingAccountId, address(wstEth), excessWithdrawAmountX18);
​
uint256 newBalance = perpsEngine.exposed_getMarginCollateralBalance(tradingAccountId, address(wstEth)).intoUint256();
console.log("New Balance: %d", newBalance);
​
assertEq(newBalance, 0, "Balance after maximum withdrawal should be 0");
​
}
​
}

Traces :

[512472] Withdraw_Unit_Test::testFuzz_AttemptWithdrawMoreThanBalance(0)
├─ [2355] wstETH::decimals() [staticcall]
│ └─ ← 18
├─ [0] console::log(Bound result, 25000000000000000 [2.5e16]) [staticcall]
│ └─ ← ()
├─ [2582] wstETH::balanceOf(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229]) [staticcall]
│ └─ ← 0
├─ [0] VM::record()
│ └─ ← ()
├─ [582] wstETH::balanceOf(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229]) [staticcall]
│ └─ ← 0
├─ [0] VM::accesses(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F])
│ └─ ← [0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc], []
├─ [0] VM::load(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc) [staticcall]
│ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
├─ emit WARNING_UninitedSlot(who: wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], slot: 17253419905260593075329270441976474840508986577177128640833442845820635365884 [1.725e76])
├─ [0] VM::load(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc) [staticcall]
│ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
├─ [582] wstETH::balanceOf(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229]) [staticcall]
│ └─ ← 0
├─ [0] VM::store(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
│ └─ ← ()
├─ [582] wstETH::balanceOf(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229]) [staticcall]
│ └─ ← 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]
├─ [0] VM::store(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc, 0x0000000000000000000000000000000000000000000000000000000000000000)
│ └─ ← ()
├─ emit SlotFound(who: wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], fsig: 0x70a08231, keysHash: 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc, slot: 17253419905260593075329270441976474840508986577177128640833442845820635365884 [1.725e76])
├─ [0] VM::load(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc) [staticcall]
│ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
├─ [0] VM::store(wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 0x262516f054714c48d5ef7386282948b73b29def7339027d51b4cdd58d9a0c1fc, 0x0000000000000000000000000000000000000000000000000058d15e17628000)
│ └─ ← ()
├─ [582] wstETH::balanceOf(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229]) [staticcall]
│ └─ ← 25000000000000000 [2.5e16]
├─ [206747] Perps Engine::createTradingAccount(0x, false)
│ ├─ [201495] TradingAccountBranch::createTradingAccount(0x, false) [delegatecall]
│ │ ├─ [126222] Trading Account NFT::mint(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1)
│ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], tokenId: 1)
│ │ │ ├─ [4585] Perps Engine::notifyAccountTransfer(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1)
│ │ │ │ ├─ [1839] TradingAccountBranch::notifyAccountTransfer(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1) [delegatecall]
│ │ │ │ │ └─ ← ()
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ emit LogCreateTradingAccount(tradingAccountId: 1, sender: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229])
│ │ └─ ← 1
│ └─ ← 1
├─ [133724] Perps Engine::depositMargin(1, wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 25000000000000000 [2.5e16])
│ ├─ [130975] TradingAccountBranch::depositMargin(1, wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 25000000000000000 [2.5e16]) [delegatecall]
│ │ ├─ [22074] wstETH::transferFrom(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], Perps Engine: [0x4C5f44874DeEF37438637949166E1761D534A9F9], 25000000000000000 [2.5e16])
│ │ │ ├─ emit Transfer(from: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], to: Perps Engine: [0x4C5f44874DeEF37438637949166E1761D534A9F9], value: 25000000000000000 [2.5e16])
│ │ │ └─ ← true
│ │ ├─ emit LogDepositMargin(sender: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], tradingAccountId: 1, collateralType: wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], amount: 25000000000000000 [2.5e16])
│ │ └─ ← ()
│ └─ ← ()
├─ [355] wstETH::decimals() [staticcall]
│ └─ ← 18
├─ [0] console::log(NEW YEAR) [staticcall]
│ └─ ← ()
├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000001374726164696e67206163636f756e743a20256400000000000000000000000000) [staticcall]
│ └─ ← ()
├─ [6820] Perps Engine::exposed_withdraw(1, wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 25000000000000002 [2.5e16])
│ ├─ [1564] TradingAccountHarness::exposed_withdraw(1, wstETH: [0x95CFFc21fa58E10C0758881CA0A61A0C6EaE774F], 25000000000000002 [2.5e16]) [delegatecall]
│ │ └─ ← "Arithmetic over/underflow"
│ └─ ← "Arithmetic over/underflow"
└─ ← "Arithmetic over/underflow"

Impact: Low

Tools Used: foundry

Recommendations: Please handle it appropiately at sub function for Arithmetic overlow/underflow.

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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