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

`Rescuable::_safe_transfer_from` function lacks thorough token transfer verification, potentially allowing successful calls without actual token transfers

guirewire

[L-1] Rescuable::_safe_transfer_from function lacks thorough token transfer verification, potentially allowing successful calls without actual token transfers

Summary:

The _safe_transfer_from function in the Rescuable contract only checks if the call to transferFrom was successful, without verifying if tokens were actually transferred. While the current risk is low due to admin control over token selection, this could potentially lead to issues if non-standard or malicious tokens are introduced to the system.

Vulnerability Details:

The _safe_transfer_from function in the Rescuable.sol contract uses a low-level call to execute the transferFrom function on the token contract:

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

This implementation only checks if the call was successful (success boolean), but doesn't verify the actual return value of transferFrom or check if tokens were moved. This could lead to a situation where the function considers a transfer successful even when no tokens have actually been transferred.

Impact:

Proof of Concept:

To demonstrate this vulnerability, we created a mock vulnerable token and a test scenario. File was named RescuableTest3.t.sol in the test directory of the project.:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import "forge-std/Test.sol";
import "../src/utils/Rescuable.sol";
​
contract VulnerableToken {
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
​
constructor() {
balanceOf[msg.sender] = 1000000 * 10 ** 18;
}
​
function approve(address spender, uint256 amount) external returns (bool) {
allowance[msg.sender][spender] = amount;
return true;
}
​
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
// Always return true, but don't actually transfer tokens
return true;
}
}
​
contract RescuableHarness is Rescuable {
function exposedSafeTransferFrom(address token, address from, address to, uint256 amount) external {
_safe_transfer_from(token, from, to, amount);
}
}
​
contract RescuableTest is Test {
RescuableHarness public rescuable;
VulnerableToken public vulnerableToken;
address public owner;
address public user;
address public attacker;
​
function setUp() public {
owner = address(this);
user = address(0x1);
attacker = address(0x2);
​
rescuable = new RescuableHarness();
vulnerableToken = new VulnerableToken();
​
// Give some tokens to the user
vulnerableToken.approve(user, 1000 * 10 ** 18);
vm.prank(user);
vulnerableToken.transferFrom(address(this), user, 1000 * 10 ** 18);
}
​
function testVulnerableTransferFrom() public {
uint256 amount = 100 * 10 ** 18;
​
// User approves Rescuable contract to spend tokens
vm.prank(user);
vulnerableToken.approve(address(rescuable), amount);
​
// Check initial balances
uint256 initialAttackerBalance = vulnerableToken.balanceOf(attacker);
uint256 initialUserBalance = vulnerableToken.balanceOf(user);
​
// Attacker calls the vulnerable function
vm.prank(attacker);
rescuable.exposedSafeTransferFrom(address(vulnerableToken), user, attacker, amount);
​
// Check final balances
uint256 finalAttackerBalance = vulnerableToken.balanceOf(attacker);
uint256 finalUserBalance = vulnerableToken.balanceOf(user);
​
// The balances shouldn't change, but the transfer is considered "successful"
assertEq(finalAttackerBalance, initialAttackerBalance, "Attacker balance should not change");
assertEq(finalUserBalance, initialUserBalance, "User balance should not change");
​
// The function didn't revert, which demonstrates the vulnerability
// In a real scenario, this could lead to incorrect state changes or further exploits
}
}

To run the test, we used the following command; forge test --match-path test/RescuableTest3.t.sol -vvvv

The test results showed that the testVulnerableTransferFrom test passed. The transferFrom function was called and returned true without actually transferring any tokens. Both balance checks (for the attacker and the user) returned 0, confirming that no actual transfer occurred, yet the function considered the transfer successful.

The current impact is low because:

  • The admin controls which ERC20 tokens are used in the system, likely choosing reputable, standard-compliant tokens.

  • The vulnerability would only be exploitable with non-standard or malicious tokens, which are unlikely to be selected by the admin.

However, if a non-standard token were to be introduced, it could potentially lead to:

  • Incorrect accounting within the protocol

  • Inconsistencies between expected and actual token balances

  • Potential for exploits in other parts of the system that rely on this function

Tools Used:

Manual review, Foundry, AI for troubleshooting

Recommended Mitigation:

While the current risk is low, improving the function can enhance the contract's robustness:

  • Use OpenZeppelin's SafeERC20 library for safer token transfers:

+ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
contract Rescuable is Ownable, Pausable {
+ using SafeERC20 for IERC20;
​
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();
- }
+ IERC20(token).safeTransferFrom(from, to, amount);
}
}

  • Alternatively, implement a balance check before and after the transfer:

function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
+ uint256 balanceBefore = IERC20(token).balanceOf(to);
​
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
​
+ uint256 balanceAfter = IERC20(token).balanceOf(to);
+ require(balanceAfter - balanceBefore == amount, "Transfer amount mismatch");
}
Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-weird-erc-20-return-boolean-Rescuable

I believe the issues and duplicates do not warrant low severity severity as even if the call to transfers returns false instead of reverting, there is no impact as it is arguably correct given there will be insufficient funds to perform a rescue/withdrawal. This will not affect `tillIn()` as there are explicit balance [checks that revert accordingly](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L255-L260) to prevent allowing creation of offers without posting the necessary collateral

Support

FAQs

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