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

Transfers would fail and the failures aren't being bubbled up

bauchibred

Summary

Transfers would fail and the failures aren't being bubbled up

Vulnerability Details

See https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L84-L117

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

Both functions are heavily used accross scope and they are meant to be helpers on transferring tokens,

Now the current implementation would not work properly ERC20s, which is because some erc20s do not revert on failure and also the fact that the return data of the transfer data is not checked on these calls

What this means is that where as the .call() here would be successful the attempt at transferring the tokens might have failed but there would be no way to bubble up this revert and this assumption would flaw the logic in the system in all the instances where these functions are being used.

Impact

Silent failures of transfer/transferFrom(), broken accounting

Tools Used

Manual review

Recommendations

Integrate a way to check if the attempted transfer on the token itself fails, integrated concept might mirror the below:

function safeTransfer(address token, address to, uint256 value) internal {
(bool success, bytes memory data) = token.call(
abi.encodeWithSelector(TRANSFER_SELECTOR, to, value)
);
require(success && (data.length == 0 || abi.decode(data, (bool))), "Transfer failed");
}
​
function safeTransferFrom(address token, address from, address to, uint256 value) internal {
(bool success, bytes memory data) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, value)
);
require(success && (data.length == 0 || abi.decode(data, (bool))), "TransferFrom failed");
}
Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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