Tadle

DeFiFoundry

27,750 USDC

View results

Previous Next

Submission Details

Severity: high

Valid

Traders will be unable to withdraw their funds

air

Summary

The TokenManager contract's withdraw function calculates the claimable amount but when transfering the amount , it encounters revert ApproveFailed() which could be may be due to insufficient permissions. This results in users being unable to fully withdraw their funds and potential accumulation of locked ETH in the contract

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137

The withdraw function determines the claimable amount from the userTokenBalanceMap. However, it then ignores this amount when performing the actual withdrawal, it encounters revert ApproveFailed(). This discrepancy potentially locking most of the user's funds in the contract.

In the CapitalPool.approve() :

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L24

If the tadleFactory.relatedContracts(RelatedContractLibraries.TOKEN_MANAGER) function returns an incorrect or uninitialized address, the approve call will fail.

The tokenAddr passed to this function must be a valid ERC20 token contract address. If it's not, the call will fail because the contract at tokenAddr may not implement the approve function

Its also has several issues :

The function fails to adjust the user's balance after withdrawal .
There is also no reentracy guard.

Proof of concept

User accumulates a balance through various transactions.
Withdrawal attempts fail due to incorrect handling in the withdraw function.

// SPDX-License-Identifier: GPL-2.0-or-later

pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";

import {SystemConfig} from "../src/core/SystemConfig.sol";

import {CapitalPool} from "../src/core/CapitalPool.sol";

import {TokenManager} from "../src/core/TokenManager.sol";

import {PreMarktes} from "../src/core/PreMarkets.sol";

import {DeliveryPlace} from "../src/core/DeliveryPlace.sol";

import {TadleFactory} from "../src/factory/TadleFactory.sol";

import {OfferStatus, StockStatus, AbortOfferStatus, OfferType, StockType, OfferSettleType} from "../src/storage/OfferStatus.sol";

import {IPerMarkets, OfferInfo, StockInfo, MakerInfo, CreateOfferParams} from "../src/interfaces/IPerMarkets.sol";

import {TokenBalanceType, ITokenManager} from "../src/interfaces/ITokenManager.sol";

import {GenerateAddress} from "../src/libraries/GenerateAddress.sol";

import {MockERC20Token} from "./mocks/MockERC20Token.sol";

import {WETH9} from "./mocks/WETH9.sol";

import {UpgradeableProxy} from "../src/proxy/UpgradeableProxy.sol";

contract Withdraw_Bug is Test {

SystemConfig systemConfig;

CapitalPool capitalPool;

TokenManager tokenManager;

PreMarktes preMarktes;

DeliveryPlace deliveryPlace;

address marketPlace;

WETH9 weth9;

MockERC20Token mockUSDCToken;

MockERC20Token mockPointToken;

address admin = vm.addr(0x123);

address user = vm.addr(0x124);

address user2 = vm.addr(0xd125);

uint256 basePlatformFeeRate = 5_000;

uint256 baseReferralRate = 300_000;

bytes4 private constant INITIALIZE_OWNERSHIP_SELECTOR =

bytes4(keccak256(bytes("initializeOwnership(address)")));

function setUp() public {

// deploy mocks

weth9 = new WETH9();

TadleFactory tadleFactory = new TadleFactory(admin);

mockUSDCToken = new MockERC20Token();

mockPointToken = new MockERC20Token();

SystemConfig systemConfigLogic = new SystemConfig();

CapitalPool capitalPoolLogic = new CapitalPool();

TokenManager tokenManagerLogic = new TokenManager();

PreMarktes preMarktesLogic = new PreMarktes();

DeliveryPlace deliveryPlaceLogic = new DeliveryPlace();

bytes memory deploy_data = abi.encodeWithSelector(

INITIALIZE_OWNERSHIP_SELECTOR,

admin

);

vm.startPrank(admin);

address systemConfigProxy = tadleFactory.deployUpgradeableProxy(

address(systemConfigLogic),

bytes(deploy_data)

);

address preMarktesProxy = tadleFactory.deployUpgradeableProxy(

address(preMarktesLogic),

bytes(deploy_data)

);

address deliveryPlaceProxy = tadleFactory.deployUpgradeableProxy(

address(deliveryPlaceLogic),

bytes(deploy_data)

);

address capitalPoolProxy = tadleFactory.deployUpgradeableProxy(

address(capitalPoolLogic),

bytes(deploy_data)

);

address tokenManagerProxy = tadleFactory.deployUpgradeableProxy(

address(tokenManagerLogic),

bytes(deploy_data)

);

// Deploy Proxy

systemConfig = SystemConfig(systemConfigProxy);

capitalPool = CapitalPool(capitalPoolProxy);

tokenManager = TokenManager(tokenManagerProxy);

preMarktes = PreMarktes(preMarktesProxy);

deliveryPlace = DeliveryPlace(deliveryPlaceProxy);

// initialize

systemConfig.initialize(basePlatformFeeRate, baseReferralRate);

tokenManager.initialize(address(weth9));

address[] memory tokenAddressList = new address[](2);

tokenAddressList[0] = address(mockUSDCToken);

tokenAddressList[1] = address(weth9);

tokenManager.updateTokenWhiteListed(tokenAddressList, true);

// create market place

systemConfig.createMarketPlace("Backpack", false);

marketPlace = GenerateAddress.generateMarketPlaceAddress("Backpack");

deal(address(mockUSDCToken), admin, 100000000 * 10 ** 18);

vm.stopPrank();

vm.warp(1719826275);

}

function test_unable_to_Withdraw () public {

// Give user some balance

deal(user, 1000);

deal(address(mockUSDCToken), user, 2000);

deal(address(weth9), address(capitalPool), 2000);

uint256 offerId = 0 ;

// Generate makerAddr Address

address makerAddr = GenerateAddress.generateMakerAddress(offerId);

// Generate offer Address

address offerAddr = GenerateAddress.generateOfferAddress(offerId);

// Generate offer Stock Address

address stockAddr = GenerateAddress.generateStockAddress(offerId);

address stock1Addr = GenerateAddress.generateStockAddress(1);

address offer1Addr = GenerateAddress.generateOfferAddress(1);

// points

uint256 _points = 500;

// amount

uint256 _amount = 63 ;

// Collateral Rate

uint256 collateralRate = 12000;

uint256 tax = 500 ;

// Create offer params

CreateOfferParams memory params = CreateOfferParams(

marketPlace,

address(mockUSDCToken),

_points,

_amount,

collateralRate,

tax,

OfferType.Ask,

OfferSettleType.Protected

);

vm.startPrank(user);

// Aprove TokenManager

mockUSDCToken.approve(address(tokenManager), type(uint256).max);

//starting user balance

uint256 startingUserBalance = mockUSDCToken.balanceOf(user);

//@audit-info Expected Event Emitted by the function CreateOffer()

vm.expectEmit(true, true, true, true , address(preMarktes));

emit CreateOffer(

offerAddr, // offer address

makerAddr, // makeraddress

stockAddr, // stock address

marketPlace, // marketPlace

user, // user

_points, // point

_amount // amount

);

// Create offer Pay

preMarktes.createOffer{value : 500}(params);

// log the user balance after creating offer

uint256 userBalanceAfterCreateOffer = mockUSDCToken.balanceOf(user);

// check if the balance has decreased

assertEq(userBalanceAfterCreateOffer, startingUserBalance - 76);

uint256 poolbalance = mockUSDCToken.balanceOf(address(capitalPool));

assertEq(poolbalance , 76);

// create taker user pays

// Ignore the eth transfer since its not accounted for

preMarktes.createTaker{value: 400}(offerAddr, 400);

uint256 poolbalance1 = mockUSDCToken.balanceOf(address(capitalPool));

assertEq(poolbalance1 , 129);

// we log the balance after creating a Taker

uint256 claimAbleAmount1 = tokenManager.userTokenBalanceMap(user, address(mockUSDCToken), TokenBalanceType.SalesRevenue);

assertEq(claimAbleAmount1 , 51);

// attempt to withdraw since withdraw is available all the time after deposit

bytes4 selector = ApproveFailed.selector;

vm.expectRevert(abi.encodeWithSelector(selector));

tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);

// create a new taker for another _point

preMarktes.createTaker(offerAddr, 100);

uint256 poolbalance2 = mockUSDCToken.balanceOf(address(capitalPool));

assertEq(poolbalance2 , 142);

// listoofer

preMarktes.listOffer(stock1Addr, _amount, collateralRate);

uint256 poolbalance3 = mockUSDCToken.balanceOf(address(capitalPool));

assertEq(poolbalance3 , 218);

// close offer

preMarktes.closeOffer(stock1Addr, offer1Addr);

uint256 poolbalance4 = mockUSDCToken.balanceOf(address(capitalPool));

assertTrue(poolbalance4 == poolbalance3);

// relist offer

preMarktes.relistOffer(stock1Addr, offer1Addr);

uint256 poolbalance5 = mockUSDCToken.balanceOf(address(capitalPool));

assertTrue(poolbalance5 > poolbalance4);

//abort offer

preMarktes.abortAskOffer(stock1Addr, offer1Addr);

// The balance has now updated

uint256 claimAbleAmount2 = tokenManager.userTokenBalanceMap(user, address(mockUSDCToken), TokenBalanceType.MakerRefund);

assertTrue(claimAbleAmount2 == 150);

// attempt to withdraw for the second time

bytes4 selector0 = ApproveFailed.selector;

vm.expectRevert(abi.encodeWithSelector(selector0));

tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);

vm.stopPrank();

}

error ApproveFailed();

// expected events

event CreateOffer(

address indexed _offer,

address indexed _maker,

address indexed _stock,

address _marketPlace,

address _authority,

uint256 _points,

uint256 _amount

);

}

run command :

forge test --mt test_unable_to_Withdraw -vvvv --via-ir

Impact

Most user funds will remain locked in the contract

Tools Used

Manual Review

Recommendations

Ensure that the contract state is correct before calling approve. For instance, the contract should have the right permissions granted to transfer funds .

Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago

Submission Judgement Published

Validated

Assigned finding tags:

finding-TokenManager-approve-wrong-address-input

If we consider the correct permissioned implementation for the `approve()` function within `CapitalPool.sol`, this would be a critical severity issue, because the withdrawal of funds will be permanently blocked and must be rescued by the admin via the `Rescuable.sol` contract, given it will always revert [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L36-L38) when attempting to call a non-existent function selector `approve` within the TokenManager contract. The argument up in the air is since the approval function `approve` was made permisionless, the `if` block within the internal `_transfer()` function will never be invoked if somebody beforehand calls approval for the TokenManager for the required token, so the transfer will infact not revert when a withdrawal is invoked. I will leave open for escalation discussions, but based on my first point, I believe high severity is appropriate.

Prize pool breakdown

Total prize

27,750 USDC

nSLOC

1,229

23 USDC / LOC

High Medium

25,000 USDC

Low

2,750 USDC

Judges

2,250 USDC

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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