hats-finance / HATs-Arbitration-Contracts-0x79a618f675857b45934ca1c413fd5f409cf89735

MIT License
0 stars 0 forks source link

After a `TokenLock` is revoked tokens can be lost permanently #54

Closed hats-bug-reporter[bot] closed 7 months ago

hats-bug-reporter[bot] commented 8 months ago

Github username: @bahurum Submission hash (on-chain): 0x237585fadf9f48a0927a8dc394c515dc1870caa41a96c96dd44d586442969686 Severity: medium

Description: Description\ In TokenLock.revoke() the unvested tokens are sent to the owner and the contract is destroyed afterwards. The issue is that the unvested tokens are only a part of the total tokens inside the TokenLock, which is a sum of:

Since the contract is destroyed after it is revoked, releasableAmount() + surplusAmount() will be stuck permanently in the TokenLock.

Recommendation\ Consider returning releasableAmount() + surplusAmount() either to the owner or the beneficiary before the contract is destroyed.

To return to the owner:

function revoke() external override onlyOwner {
    if (!revocable)
        revert LockIsNonRevocable();

-   uint256 unvestedAmount = managedAmount - vestedAmount();
+   uint256 revokedAmount = token.balanceOf(address(this));
-   if (unvestedAmount == 0)
+   if (revokedAmount == 0)
        revert NoAvailableUnvestedAmount();

    isRevoked = true;

-   token.safeTransfer(owner(), unvestedAmount);
+   token.safeTransfer(owner(), revokedAmount);

-   emit TokensRevoked(beneficiary, unvestedAmount);
+   emit TokensRevoked(beneficiary, revokedAmount);

    selfdestruct(payable(msg.sender));
}

To return to the beneficiary:

function revoke() external override onlyOwner {
    if (!revocable)
        revert LockIsNonRevocable();

    uint256 unvestedAmount = managedAmount - vestedAmount();
    if (unvestedAmount == 0)
        revert NoAvailableUnvestedAmount();

    isRevoked = true;

+   token.safeTransfer(beneficiary, `releasableAmount() + surplusAmount()`);
    token.safeTransfer(owner(), unvestedAmount);

    emit TokensRevoked(beneficiary, unvestedAmount);

    selfdestruct(payable(msg.sender));
}