Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`withdraw()` function wont work when trying to withdraw wrapped Native Tokens

adeolu

Summary

withdraw() function wont work when trying to withdraw wrapped Native Tokens as wrong argument is supplied as token address for capitalPool.approve() which is called later in the withdraw() function logic via the internal _transfer() function.

Vulnerability Details

tokenManager.withdraw() will attempt to withdraw wrapped native tokens from the capital pool before converting them to ETH and sending that to the caller. The pull of tokens from the capital pool will occur via the internal _transfer() function.
In the _transfer() function, before a safeTransferFrom is used by tokenManager to take the tokens from the capital pool, _capitalPool.approve is called so that tokenManager is granted permission to take these tokens from the capital pool. But it is called with a wrong parameter in tokenManager._transfer(). The function _capitalPool.approve() ought to be called with a token address as parameter as specified by the function's selector. _capitalPool.approve() snippet below.

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

/**
* @dev Approve token for token manager
* @notice only can be called by token manager
* @param tokenAddr address of token
*/
function approve(address tokenAddr) external {

But instead, tokenManager._transfer() supplies its own address when calling the _capitalPool.approve() function (See snippet below). Token manager is not an erc20 token so it doesn't even have an approve function.

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

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this)); //@audit why is address(this) the token address? address(this) is the token manager and not the token to be transferred
}

The call to _capitalPool.approve does cause the whole execution to revert with an ApproveFailed() error as the function approve() does not exist on tokenManager's address which was supplied to the _capitalPool.approve() fcn. Snippet of _capitalPool.approvebelow showing the required parameter for the function.

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

/* @param tokenAddr address of token
*/
function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

Proof Of Concept

The POC test below fails with this revert statement below

│ │ │ │ │ ├─ [192] TokenManager::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]) [delegatecall]
│ │ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ └─ ← [Revert] ApproveFailed()
│ │ │ └─ ← [Revert] ApproveFailed()
│ │ └─ ← [Revert] ApproveFailed()
│ └─ ← [Revert] ApproveFailed()
└─ ← [Revert] ApproveFailed()
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.31ms (448.75µs CPU time)
​
Ran 1 test suite in 1.07s (9.31ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/testCase01.t.sol:TokenManagerTest
[FAIL. Reason: ApproveFailed()] test_capitalPoolApproveFails() (gas: 92174)

To run the POC test:

  • paste code in new file in /test folder

  • run with fforge test --mt test_capitalPoolApproveFails -vvv

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.13;
​
import {Test, console} 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 {WETH9} from "./mocks/WETH9.sol";
import {UpgradeableProxy} from "../src/proxy/UpgradeableProxy.sol";
import {MockERC20Token} from "./mocks/MockERC20Token.sol";
​
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract TokenManagerTest is Test {
SystemConfig systemConfig;
CapitalPool capitalPool;
TokenManager tokenManager;
PreMarktes preMarkets;
DeliveryPlace deliveryPlace;
​
address marketPlace;
WETH9 weth9;
MockERC20Token mockUSDCToken;
MockERC20Token mockPointToken;
MockERC20Token newERC20TokenToBeAdded;
​
address user = vm.addr(1);
address user1 = vm.addr(2);
address user2 = vm.addr(3);
address user3 = vm.addr(4);
​
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(user1);
​
mockUSDCToken = new MockERC20Token();
mockPointToken = new MockERC20Token();
newERC20TokenToBeAdded = new MockERC20Token();
​
SystemConfig systemConfigLogic = new SystemConfig();
CapitalPool capitalPoolLogic = new CapitalPool();
TokenManager tokenManagerLogic = new TokenManager();
PreMarktes preMarketsLogic = new PreMarktes();
DeliveryPlace deliveryPlaceLogic = new DeliveryPlace();
​
bytes memory deploy_data = abi.encodeWithSelector(
INITIALIZE_OWNERSHIP_SELECTOR,
user1
);
vm.startPrank(user1);
​
address systemConfigProxy = tadleFactory.deployUpgradeableProxy(
1,
address(systemConfigLogic),
bytes(deploy_data)
);
​
address preMarketsProxy = tadleFactory.deployUpgradeableProxy(
2,
address(preMarketsLogic),
bytes(deploy_data)
);
address deliveryPlaceProxy = tadleFactory.deployUpgradeableProxy(
3,
address(deliveryPlaceLogic),
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
systemConfig = SystemConfig(systemConfigProxy);
capitalPool = CapitalPool(capitalPoolProxy);
tokenManager = TokenManager(tokenManagerProxy);
preMarkets = PreMarktes(preMarketsProxy);
deliveryPlace = DeliveryPlace(deliveryPlaceProxy);
​
vm.startPrank(user1);
vm.deal(user1, 10 ether);
​
weth9.deposit{value: 1 ether}();
weth9.transfer(address(capitalPool), 1 ether);
​
// initialize
systemConfig.initialize(basePlatformFeeRate, baseReferralRate);
tokenManager.initialize(address(weth9));
address[] memory tokenAddressList = new address[](3);
​
tokenAddressList[0] = address(mockUSDCToken);
tokenAddressList[1] = address(weth9);
tokenAddressList[2] = address(newERC20TokenToBeAdded);
​
tokenManager.updateTokenWhiteListed(tokenAddressList, true);
​
// create market place
systemConfig.createMarketPlace("Backpack", false);
vm.stopPrank();
​
deal(address(mockUSDCToken), user, 100000000 * 10 ** 18);
deal(address(mockPointToken), user, 100000000 * 10 ** 18);
deal(address(newERC20TokenToBeAdded), user, 100000000 * 10 ** 18);
deal(
address(newERC20TokenToBeAdded),
address(capitalPool),
100000000 * 10 ** 18
);
​
deal(user, 100000000 * 10 ** 18);
​
deal(address(mockUSDCToken), user1, 100000000 * 10 ** 18);
deal(address(mockUSDCToken), user2, 100000000 * 10 ** 18);
deal(address(mockUSDCToken), user3, 100000000 * 10 ** 18);
​
deal(address(mockPointToken), user2, 100000000 * 10 ** 18);
​
deal(address(mockPointToken), address(capitalPool), 1000 * 10 ** 18);
deal(address(mockUSDCToken), address(capitalPool), 1000 * 10 ** 18);
​
marketPlace = GenerateAddress.generateMarketPlaceAddress("Backpack");
​
vm.warp(1719826275);
​
vm.prank(user);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
mockPointToken.approve(address(tokenManager), type(uint256).max);
vm.stopPrank();
}
​
function test_capitalPoolApproveFails() public {
vm.startPrank(address(preMarkets));
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
user1,
address(weth9),
0.5 ether
);
​
//now user tries to withdraw with wrapped eth
vm.startPrank(address(user1));
//this fails because the call by token manager to capitalPool.approve() does not pass in the weth9 token address
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
vm.stopPrank();
}
}
​

Impact

inability of users to withdraw claimabale amount as ETH because of wrong token address being used as parameter in an external function call to _capitalPool.approve()

Tools Used

foundry

Recommended Mitigation Steps

pass in the token address in tokenManager._transfer() like below

ICapitalPool(_capitalPoolAddr).approve(_token);
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year 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.

Support

FAQs

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