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

The `TokenManager::_transfer()` function incorrectly uses `address(this)` as the parameter for the `CapitalPool::approve()` function.

joicygiore

Summary

The TokenManager::_transfer() function incorrectly uses address(this) as the parameter for the CapitalPool::approve() function.

Vulnerability Details

In the TokenManager::_transfer() function, when the IERC20(_token).allowance(_from, address(this)) == 0x0 contract authorization allowance is equal to 0, the CapitalPool::approve() function is called to authorize the TokenManager contract allowance. The CapitalPool::approve() function's implementation calls the tokenAddr::approve() function to authorize the tokenManager address allowance. Therefore, using address(this) as the calling parameter is incorrect.

// TokenManager::_transfer()
function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
​
if (
_from == _capitalPoolAddr &&
@> IERC20(_token).allowance(_from, address(this)) == 0x0
) {
@> ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
@> _safe_transfer_from(_token, _from, _to, _amount);
// SNIP...
}
​
// CapitalPool::approve()
function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
@> (bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR, // APPROVE_SELECTOR = bytes4(keccak256(bytes("approve(address,uint256)")));
@> tokenManager,
type(uint256).max
)
);
​
​
if (!success) {
revert ApproveFailed();
}
}

We can see that there is no approve(address,uint256) related function in the TokenManager contract

{
addTokenBalance(uint8,address,address,uint256): "94e73d41",
initialize(address): "c4d66de8",
initializeOwnership(address): "5cd9205f",
owner(): "8da5cb5b",
paused(): "5c975abb",
renounceOwnership(): "715018a6",
rescue(address,address,uint256): "20ff430b",
setPauseStatus(bool): "c38bb537",
tadleFactory(): "8ef1d7b4",
tillIn(address,address,uint256,bool): "8e506349",
tokenWhiteListed(address): "c00dc402",
transferOwnership(address): "f2fde38b",
updateTokenWhiteListed(address[],bool): "431b4c07",
userTokenBalanceMap(address,address,uint8): "fab3fef5",
withdraw(address,uint8): "cc5a02cb",
wrappedNativeToken(): "17fcb39b"
}

Poc

Please add the test code to test/PreMarkets.t.sol and execute

import {ICapitalPool} from "../src/interfaces/ICapitalPool.sol";
function testTokenManager__transfer_error() public {
// Use eth for testing, forge eth funds for use2
deal(user2, 1e18);
​
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.12 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
//
vm.prank(user2);
preMarktes.createTaker{value: 0.05175 * 1e18}(offerAddr, 500);
vm.stopPrank();
​
// get user SalesRevenue
uint256 userSalesRevenue = tokenManager.userTokenBalanceMap(
address(user),
address(weth9),
TokenBalanceType.SalesRevenue
);
console2.log(userSalesRevenue);
​
// user calls withdraw
vm.startPrank(user);
// We can see that it is restored when executing `TokenManager::_transfer() -> ICapitalPool(_capitalPoolAddr).approve(address(this));`
vm.expectRevert(ICapitalPool.ApproveFailed.selector); // ApproveFailed()
tokenManager.withdraw(address(weth9),TokenBalanceType.SalesRevenue);
}
// Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
// [PASS] testTokenManager__transfer_error() (gas: 888044)

Code Snippet

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

Impact

The TokenManager::_transfer() function incorrectly uses address(this) as the parameter for the CapitalPool::approve() function, which should be _token.

Tools Used

Manual Review

Recommendations

Modify the parameter to _token

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
​
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(address(_token));
}
// SNIP...
}
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.

Support

FAQs

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