Tadle

DeFi

30,000 USDC

View results

Previous Next

Submission Details

Severity: high

Valid

Loss Of Funds Due To Unreachable Allowance Check For Non Native Withdraw Tokens

kamensec

Summary

The TokenManager contract does not perform an allowance check when _tokenAddress != wrappedNativeToken during withdrawals, which results in failed withdrawals for non-wrapped native tokens.

Vulnerability Details

Overview

The _transfer() function in the TokenManager contract is supposed to handle token transfers and allowance checks. However, when _tokenAddress != wrappedNativeToken, the _safe_transfer_from() is used instead as can be seen here. This function does not perform equivalent checks for allowance, and the following block of code is unreachable for all non-wrapped native tokens.

Deposits to the protocol are successful, but withdrawals will fail due to revert ERC20InsufficientAllowance. This will prevent all users from withdrawing their tokens.

NOTE: It is worth noting that when tillIn() is used with non nativeWrappedToken this will reach the if condition in the _transfer() function. However, since the _from != _capitalPoolAddr the allowance will not be incremented.

Proof of Concept

Please run the following test to reproduce the issue: forge test --via-ir --mt testCloseBidOfferBalances

// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.13;

import "forge-std/Test.sol";

import "../src/core/PreMarkets.sol";

import "../src/libraries/GenerateAddress.sol";

import "../src/libraries/Constants.sol";

import "../src/interfaces/ITokenManager.sol";

import "../src/interfaces/ISystemConfig.sol";

import {MockERC20Token} from "./mocks/MockERC20Token.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 {WETH9} from "./mocks/WETH9.sol";

contract IssueTestTemplate is Test {

SystemConfig systemConfig;

CapitalPool capitalPool;

TokenManager tokenManager;

PreMarktes preMarktes;

DeliveryPlace deliveryPlace;

address marketPlace;

string marketPlaceName = "Backpack";

WETH9 weth9;

MockERC20Token mockUSDCToken;

MockERC20Token mockPointToken;

address guardian;

address maker;

address taker1;

address taker2;

address taker3;

uint256 basePlatformFeeRate = 5_000;

uint256 baseReferralRate = 300_000;

bytes4 private constant INITIALIZE_OWNERSHIP_SELECTOR =

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

function setUp() public {

// Set up accounts

guardian = makeAddr("guardian");

maker = makeAddr("maker");

taker1 = makeAddr("taker1");

taker2 = makeAddr("taker2");

taker3 = makeAddr("taker3");

vm.label(guardian, "guardian");

vm.label(maker, "maker");

vm.label(taker1, "taker1");

vm.label(taker2, "taker2");

vm.label(taker3, "taker3");

// deploy mocks

weth9 = new WETH9();

TadleFactory tadleFactory = new TadleFactory(guardian);

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,

guardian

);

vm.startPrank(guardian);

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)

);

vm.label(systemConfigProxy, "systemConfigProxy");

vm.label(preMarktesProxy, "preMarktesProxy");

vm.label(deliveryPlaceProxy, "deliveryPlaceProxy");

vm.label(capitalPoolProxy, "capitalPoolProxy");

vm.label(tokenManagerProxy, "tokenManagerProxy");

vm.stopPrank();

// attach logic

systemConfig = SystemConfig(systemConfigProxy);

capitalPool = CapitalPool(capitalPoolProxy);

tokenManager = TokenManager(tokenManagerProxy);

preMarktes = PreMarktes(preMarktesProxy);

deliveryPlace = DeliveryPlace(deliveryPlaceProxy);

vm.label(address(systemConfig), "systemConfig");

vm.label(address(tokenManager), "tokenManager");

vm.label(address(preMarktes), "preMarktes");

vm.label(address(deliveryPlace), "deliveryPlace");

vm.startPrank(guardian);

// 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(marketPlaceName, false);

vm.stopPrank();

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

deal(address(mockPointToken), maker, 100000000 * 10 ** 18);

deal(maker, 100000000 * 10 ** 18);

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

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

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

deal(address(mockPointToken), taker2, 100000000 * 10 ** 18);

marketPlace = GenerateAddress.generateMarketPlaceAddress("Backpack");

vm.warp(1719826275);

vm.prank(maker);

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

vm.startPrank(taker2);

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

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

vm.stopPrank();

}

/* @audit - POC: [allowance-check-unreachable.md]

If no native wrapped tokens are traded on the platform, the `_transfer()` block will not be reached.

As a result no allowance checks are made, which means that deposits are allowed but withdrawals will fail

due to reverts `ERC20InsufficientAllowance`.

function testCloseBidOfferBalances() public {

// Setup

uint256 totalPoints = 1000;

uint256 tokenAmount = 1e18;

uint256 collateralRate = 12000;

uint256 tradeTax = 300;

uint256 makerInitialBalance = mockUSDCToken.balanceOf(maker);

// Create a bid offer

vm.startPrank(maker);

preMarktes.createOffer(

CreateOfferParams(

marketPlace,

address(mockUSDCToken),

totalPoints,

tokenAmount,

collateralRate,

tradeTax,

OfferType.Bid,

OfferSettleType.Turbo

)

);

vm.stopPrank();

address offerAddr = GenerateAddress.generateOfferAddress(0);

// Get initial balances

uint256 makerInitialRefundBalance = getAccountTypeBalance(address(mockUSDCToken), maker, TokenBalanceType.MakerRefund);

// Calculate expected refund

uint256 collateralAmount = (tokenAmount * collateralRate) / Constants.COLLATERAL_RATE_DECIMAL_SCALER;

uint256 expectedRefund = collateralAmount * totalPoints;

// Warp time to enter AskSettling state

vm.prank(guardian);

systemConfig.updateMarket(

"Backpack",

address(mockPointToken),

tokenAmount,

block.timestamp - 1,

3600

);

// Close bid offer

vm.prank(maker);

deliveryPlace.closeBidOffer(offerAddr);

// Verify offer status

(OfferInfo memory offerInfo, , , ) = deliveryPlace.getOfferInfo(offerAddr);

assertEq(uint(offerInfo.offerStatus), uint(OfferStatus.Settled), "Offer status should be Settled");

vm.startPrank(maker);

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

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

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

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

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

vm.stopPrank();

uint256 makerFinalActualBalance = mockUSDCToken.balanceOf(maker);

assertEq(makerFinalActualBalance, makerInitialBalance, "Incorrect maker final actual balance after withdrawal");

}

// Helper functions

function createOffer(address offerer, address tokenAddress, uint256 totalPoints, uint256 tokenAmount, uint256 collateralRate, uint256 tradeTax, OfferType offerType, OfferSettleType settleType) internal returns (address offerAddr) {

vm.startPrank(offerer);

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

preMarktes.createOffer(

CreateOfferParams(

marketPlace,

tokenAddress,

totalPoints,

tokenAmount,

collateralRate,

tradeTax,

offerType,

settleType

)

);

offerAddr = GenerateAddress.generateOfferAddress(0);

vm.stopPrank();

return offerAddr;

}

function createTakerAndGetInitialBalances(address referrer, address offerAddr, uint256 _purchasedPoints) internal returns (uint256 initialReferrerBalance, uint256 initialTakerBalance) {

initialReferrerBalance = tokenManager.userTokenBalanceMap(

referrer,

address(mockUSDCToken),

TokenBalanceType.ReferralBonus

);

initialTakerBalance = tokenManager.userTokenBalanceMap(

taker1,

address(mockUSDCToken),

TokenBalanceType.ReferralBonus

);

vm.startPrank(taker1);

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

uint256 purchasedPoints = _purchasedPoints;

preMarktes.createTaker(offerAddr, purchasedPoints);

vm.stopPrank();

return (initialReferrerBalance, initialTakerBalance);

}

function closeOffer(address offerer, address offerAddr) internal {

vm.prank(offerer);

preMarktes.closeOffer(GenerateAddress.generateStockAddress(0), offerAddr);

}

function abortAskOffer(address offerer, address _stock, address _offer) internal {

vm.prank(offerer);

preMarktes.abortAskOffer(_stock, _offer);

}

function abortBidTaker(address offerer, address _stock, address _offer) internal {

vm.prank(offerer);

preMarktes.abortBidTaker(_stock, _offer);

}

function abortBidTakerFixed(address offerer, address _stock, address _offer) internal {

vm.prank(offerer);

preMarktes.abortBidTakerFixed(_stock, _offer);

}

function verifyAccountTypeBalance(address _token, address account, TokenBalanceType accountType, uint256 expectedBalance) internal returns(uint256 balance) {

balance = getAccountTypeBalance(_token, account, accountType);

console2.log("AccountType", uint256(accountType));

assertEq(

balance,

expectedBalance,

"Account should have the correct amount for account type"

);

return balance;

}

function getAccountTypeBalance(address _token, address account, TokenBalanceType accountType) public returns(uint256 balance) {

balance = tokenManager.userTokenBalanceMap(

account,

_token,

accountType

);

return balance;

}

function calculateExpectedReferralBonuses(address offerAddr, uint256 offerPoints, uint256 offerAmount, uint256 purchasedPoints) internal view returns (uint256 expectedReferrerBonus, uint256 expectedTakerBonus) {

OfferInfo memory offerInfo = preMarktes.getOfferInfo(offerAddr);

uint256 depositAmount = purchasedPoints * offerAmount / offerPoints;

uint256 platformFeeRate = systemConfig.getPlatformFeeRate(taker1);

uint256 platformFee = depositAmount * platformFeeRate / Constants.PLATFORM_FEE_DECIMAL_SCALER;

ReferralInfo memory referralInfo = systemConfig.getReferralInfo(taker1);

expectedReferrerBonus = (platformFee * referralInfo.referrerRate) / Constants.REFERRAL_RATE_DECIMAL_SCALER;

expectedTakerBonus = (platformFee * referralInfo.authorityRate) / Constants.REFERRAL_RATE_DECIMAL_SCALER;

return (expectedReferrerBonus, expectedTakerBonus);

}

function calculateExpectedSalesRevenue(address offerAddr, uint256 purchasedPoints) internal view returns (uint256 expectedSalesRevenue) {

OfferInfo memory offerInfo = preMarktes.getOfferInfo(offerAddr);

expectedSalesRevenue = purchasedPoints * offerInfo.amount / offerInfo.points;

return expectedSalesRevenue;

}

function calculateExpectedTaxIncome(address offerAddr, bool isMaker, address _maker, uint256 depositAmount) internal view returns (uint256 expectedTaxIncome) {

uint256 tradeTax = isMaker? preMarktes.getMakerInfo(_maker).eachTradeTax : preMarktes.getOfferInfo(offerAddr).tradeTax;

assertGt(tradeTax, 0, "Trade tax should be greater than 0");

expectedTaxIncome = (depositAmount * tradeTax) / Constants.EACH_TRADE_TAX_DECIMAL_SCALER;

return expectedTaxIncome;

}

Impact

Likelihood: HIGH
Impact: HIGH
Severity: HIGH
Complete loss of funds for all users and requires complete contract upgrades to rescue funds (even Rescuable contract will fail as the TokenManager contract uses those functions in Rescuable contract to perform the transfer in the first place).

Tools Used

Manual review and Forge Foundry testing

Recommendations

Ensure that the allowance check is performed for all token addresses, not just wrapped native tokens. This can be done by either removing the _from == _capitalPoolAddr condition in the _transfer() function or adding an allowance check for non-wrapped native tokens in the Rescuable._safe_transfer_from function as follows:

if (

_from == _capitalPoolAddr &&

IERC20(_token).allowance(_from, address(this)) == 0x0

) {

ICapitalPool(_capitalPoolAddr).approve(address(_token));

}

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago

Submission Judgement Published

Validated

Assigned finding tags:

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby 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. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, 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. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Prize pool breakdown

Total prize

30,000 USDC

nSLOC

1,229

24 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.