Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

The protocol is incompatible with `ZKsync Era` chain

bube

Summary

Several functions in the protocol call Rescuable::_safe_transfer or Rescuable::_safe_transfer_from functions. But these functions will not work properly on ZKsync Era chain.

Vulnerability Details

The TokenManager::tillIn, TokenManager::withdraw and TokenManager::_transfer functions rely on Rescuable::_safe_transfer or Rescuable::_safe_transfer_from functions:

TokenManager::tillIn:

​
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);
}
​

TokenManager::withdraw:

​
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
@> _transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
@> _safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​

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);
​
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}
​

The Rescuable contract is not in scope but the _safe_transfer and _safe_transfer_from functions are used in scope in very important for the protocol functionality functions. So the issues related to these two functions will break the protocol functionality.
Let's consider the _safe_transfer and _safe_transfer_from functions:

​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
@> (bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}
​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
@> (bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}
​

The purpose of these functions is to transfer a given token or ethers. According to the documentation of the contest the protocol should be compatible with every EVM-compatible chain:

Compatibilities:
Blockchains:
- Ethereum/Any EVM

The problem is that the _safe_transfer and _safe_transfer_from functions transfer tokens or ethers using the call function. But the way in which they use the call function will not work on ZKsync chain. The call on ZKsync chain works differently according to the docs:
https://docs.zksync.io/build/developer-reference/ethereum-differences/evm-instructions#call-staticcall-delegatecall

For calls, you specify a memory slice to write the return data to, e.g. out and outsize arguments for call(g, a, v, in, insize, out, outsize). In EVM, if outsize != 0, the allocated memory will grow to out + outsize (rounded up to the words) regardless of the returndatasize. On ZKsync Era, returndatacopy, similar to calldatacopy, is implemented as a cycle iterating over return data with a few additional checks and triggering a panic if out + outsize > returndatasize to simulate the same behavior as in EVM.
Thus, unlike EVM where memory growth occurs before the call itself, on ZKsync Era, the necessary copying of return data happens only after the call has ended, leading to a difference in msize() and sometimes ZKsync Era not panicking where EVM would panic due to the difference in memory growth.
Additionally, there is no native support for passing Ether on ZKsync Era, so it is handled by a special system contract called MsgValueSimulator. The simulator receives the callee address and Ether amount, performs all necessary balance changes, and then calls the callee.

Let's take a look on a differences between call on Ethereum and on ZKsync:

In Ethereum when using call(gas(), target, value, in, insize, out, outsize), the EVM will allocate memory up to out + outsize regardless of the actual size of - the returned data (returndatasize). This allocation happens before the call is made. If outsize is non-zero, the memory growth occurs immediately.

success := call(gas(), target, 0, in, insize, out, outsize) // grows to 'min(returndatasize(), out + outsize)'

In ZKsync the memory allocation is performed for return data only after the call ends. Instead of pre-allocating memory, ZKsync Era copies the return data with additional checks to ensure that out + outsize does not exceed returndatasize. This means msize() (memory size) differs between ZKsync Era and EVM during the call, and ZKsync might not trigger a panic in scenarios where EVM would, due to the difference in memory growth timing.

bool success = call(gas(), target, 0, in, insize, out, 0);
if (!success) revert("Call failed");
returndatacopy(out, 0, returndatasize());

Also, if the functions should work with ethers like the comments describe, that will be impossible on ZKsync. That is because there is no native support for passing Ether on ZKsync Era, so it is handled by a special system contract called MsgValueSimulator.

Impact

The different behaviour of call method on ZKsync Era chain could lead to failed transactions and loss funds.
Certain calls that should fail due to excessive memory usage might initially appear successful but will eventually fail due to an EVM error. Conversely, because the _safe_transfer and _safe_transfer_from functions store returned data in memory, some operations that should succeed might end up failing.

Tools Used

Manual Review

Recommendations

Use the assembly call instead of the standard .call that limits the return data to length 0.

​
function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
- (bool success, ) = token.call(
- abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
- );
​
+ bytes memory data = abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount);
+ bool success;
​
+ assembly {
+ success := call(gas(), token, 0, add(data, 0x20), mload(data), 0, 0)
+ }
​
if (!success) {
revert TransferFailed();
}
}
​
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
- (bool success, ) = token.call(
- abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
- );
​
+ bytes memory data = abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount);
+ bool success;
​
+ assembly {
+ success := call(gas(), token, 0, add(data, 0x20), mload(data), 0, 0)
+ }
if (!success) {
revert TransferFailed();
}
}
​
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other
Assigned finding tags:

[invalid] finding-zkSync-low-level-call

I would require a more explicit example on zkSync mainnet to prove that such a low level call is indeed an issue, i.e. a example live contract on zkSync that shows the usual low level `call()` cannot be utilized.

Support

FAQs

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