Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

All ETH sent while tilling-in and also specifying an ERC20 token address will be lost to the depositor

0xswahili

Summary

The TokenManager::tillin function does not check that the depositor has sent ETH as well as an ERC20 amount.

Vulnerability Details

While tilling in, users can deposit either native tokens or ERC20 tokens. For instance, makers can pay either in ETH or ERC20 token:

function createOffer(CreateOfferParams calldata params) external payable {
...
​
​
{
/// @dev transfer collateral from _msgSender() to capital pool
uint256 transferAmount = OfferLibraries.getDepositAmount(
params.offerType,
params.collateralRate,
params.amount,
true,
Math.Rounding.Ceil
);
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.tillIn{value: msg.value}(
_msgSender(),
params.tokenAddress,
transferAmount,
false
);
}
​
...
}

However the vulnerability exists in the tillin function:

/**
* @notice Till in, Transfer token from msg sender to capital pool
* @param _accountAddress Account address
* @param _tokenAddress Token address
* @param _amount Transfer amount
* @param _isPointToken The transfer token is pointToken
* @notice Capital pool should be deployed
* @dev Support ERC20 token and native token
*/
function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
/// @notice return if amount is 0
if (_amount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
if (capitalPoolAddr == address(0x0)) {
revert Errors.ContractIsNotDeployed();
}
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @notice check msg value
* @dev if msg value is less than _amount, revert
* @dev wrap native token and transfer to capital pool
*/
if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
}

The function does not check that the depositor has sent ETH together with an ERC20 amount. Assuming for example that the depositor has sent ETH and also specified the USDC token address, the TokenManager contract will pull the expected USDC amount from the user account and also receive ETH. The paid-in ETH unfortunately will never be credited to the user account.

While it is true that this vulnerability can be considered as user-input oriented, the protocol should take measure to prevent such accidental incidences now that we're aware of such possiblity. This vulnerability can be exploited where the tillin function is called, including:

  • createOffer

  • relistOffer

  • listOffer

  • createTaker

Consider this POC that shows that ETH sent together with an ERC20 token will be lost.

function test_ether_steal_protected_offer() public {
vm.startPrank(user);
uint256 usdcBal1 = mockUSDCToken.balanceOf(address(user));
uint256 ethBal1 = user.balance;
preMarktes.createOffer{value: 20_000}(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
uint256 usdcBal2 = mockUSDCToken.balanceOf(address(user));
uint256 ethBal2 = user.balance;
assertGt(usdcBal1,usdcBal2);
assertGt(ethBal1,ethBal2);
}

Impact

Depositors who sent ETH while also specifying an ERC20 address will lose thier ETH

Tools Used

Manual review

Recommendations

Consider checking that depositor has NOT sent ETH and also specified an ERC20 amount:

} else {
if (msg.value > 0 ) {
revert Errors.EthAndERC20Sent(msg.value, _amount);
}
/// @notice token is ERC20 token
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-TokenManager-tillin-excess

Invalid, these are by default, invalid based on codehawks [general guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). The check implemented is simply a sufficiency check, it is users responsibility to only send an appropriate amount of native tokens where amount == msg.value when native token is intended to be used as collateral (which will subsequently be deposited as wrapped token). All excess ETH can be rescued using the `Rescuable.sol` contract. > Users sending ETH/native tokens > If contracts allow users to send tokens acc111identally.

Support

FAQs

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