062.md

Glamorous Plum Baboon

Medium

Lack of Limits or Validation on amount Parameter in rescueTokens Function

Summary

The rescueTokens function in the contract allows a privileged user (the onlyPoolAdmin) to rescue tokens from the pool by specifying the token, recipient, and amount. However, the amount parameter lacks adequate limits or validation, allowing an admin to rescue all tokens from the contract, including tokens intended for reserves or other purposes. This issue introduces the risk of centralized abuse or accidental mismanagement of funds.

Description

The rescueTokens function is designed to provide a mechanism for administrators to recover mistakenly sent or trapped tokens from the pool. While the intention of the function is valid, the implementation does not enforce any constraints or validations on the amount parameter. As a result:

1. Unintended Risks: An admin could inadvertently rescue all tokens of a specific type, even those critical to the protocol's operation (e.g., reserve tokens or liquidity pool assets).
2. Centralized Abuse: A malicious admin could misuse this functionality to drain funds from the pool.

The issue stems from the absence of logic to validate the type of token and the maximum amount that can be rescued.

Root Cause

The function does not validate the amount parameter or ensure that the tokens being rescued are non-essential to the protocol's functionality. The relevant line of code (LoC) is as follows:

PoolLogic.executeRescueTokens(token, to, amount);

https://github.com/sherlock-audit/2025-01-aave-v3-3/blob/main/aave-v3-origin/src/contracts/protocol/pool/Pool.sol#L814-L820

This line directly calls the executeRescueTokens function in the PoolLogic contract without any prior checks to ensure that:

1. The amount is within reasonable limits.
2. The token being rescued is not a reserve or critical asset.

Impact

The lack of validation exposes the protocol to two main risks:

1. Fund Mismanagement: An admin could accidentally rescue all tokens of a given type, disrupting the protocol's liquidity or operations.
2. Centralized Abuse: A malicious admin could intentionally drain all tokens from the contract, leading to significant financial loss for users.

This issue increases the protocol's exposure to both intentional and accidental misuse by privileged actors.

Proof of Concept (PoC)

Consider the following scenario:

1. Assume the contract holds 1,000 USDC as reserves for liquidity purposes.
2. The onlyPoolAdmin executes the following call:

   rescueTokens(USDC, adminAddress, 1000);

3. All 1,000 USDC tokens are transferred to the admin’s address, leaving the protocol without reserves.

The lack of validation allows such a call to succeed, demonstrating the issue.

Test for rescueTokens Vulnerability

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "openzeppelin-contracts/token/ERC20/ERC20.sol";

contract RescueTokensTest is Test {
    YourContract public contractInstance;
    ERC20 public usdc;
    address public admin = address(0xABCD); // Admin address
    address public recipient = address(0x1234); // Recipient address

    function setUp() public {
        // Deploy mock USDC token
        usdc = new ERC20("Mock USDC", "USDC");
        
        // Deploy the main contract and set the admin
        contractInstance = new YourContract();
        contractInstance.setPoolAdmin(admin);

        // Mint 1,000 USDC to the contract
        vm.startPrank(admin); // Ensure admin calls the setup
        usdc.transfer(address(contractInstance), 1000 ether);
        vm.stopPrank();
    }

    function testRescueTokensVulnerability() public {
        // Assert that the contract initially holds 1,000 USDC
        assertEq(usdc.balanceOf(address(contractInstance)), 1000 ether);

        // Admin rescues 1,000 USDC
        vm.startPrank(admin);
        contractInstance.rescueTokens(address(usdc), recipient, 1000 ether);
        vm.stopPrank();

        // Assert that the recipient now holds the rescued 1,000 USDC
        assertEq(usdc.balanceOf(recipient), 1000 ether);

        // Assert that the contract's USDC balance is now zero
        assertEq(usdc.balanceOf(address(contractInstance)), 0);
    }
}

NB: import your contract path to the test, i forgot to add mine

Explanation:

1. Setup Phase (setUp):

   • Deploys a mock USDC token using OpenZeppelin's ERC20 implementation.
   • Deploys the main contract (YourContract) and sets the admin.
   • Mints and transfers 1,000 USDC to the contract.

2. Test Case (testRescueTokensVulnerability):

   • Ensures the contract initially holds 1,000 USDC.
   • Simulates the rescueTokens call by the admin to rescue all 1,000 USDC.
   • Asserts that the recipient receives the 1,000 USDC.
   • Validates that the contract's balance of USDC is reduced to zero, leaving it without reserves.

3. Outcome:

   • This test highlights the vulnerability by showing that the admin can transfer all tokens without any validation, leaving the contract devoid of reserves.

Recommended Mitigation

To address this vulnerability, the following changes are recommended:

1. Validate the amount Parameter:
   Add logic to ensure that the amount parameter does not exceed a predefined limit or percentage of the token's balance in the contract.

   Example:

   require(amount <= token.balanceOf(address(this)) / 10, "Exceeds rescue limit");

2. Restrict Token Types:
   Maintain a whitelist or blacklist of tokens that can be rescued. For instance, reserve tokens critical to the protocol’s functionality should not be rescueable.

3. Review by Multi-Sig:
   Require additional checks, such as approvals by multiple administrators, before executing the rescueTokens function.

By implementing these measures, the protocol can significantly reduce the risk of abuse or accidental mismanagement while retaining the ability to rescue tokens when necessary.