jasonxiale - `VestingEscrow.revokeAll` doesn't conform with `VestingEscrow.claim`

[sherlock-admin2]

jasonxiale

medium

VestingEscrow.revokeAll doesn't conform with VestingEscrow.claim

Summary

In VestingEscrow.claim, when there are unclaimed token available the recipient can choose which address to receive the token by beneficiary parameter, it means the token can still under recipient's possess. But in VestingEscrow.revokeAll, the unclaimed will be sent to the owner together with locked token, which means the unclaimed token will under owner's possess.

Vulnerability Detail

In VestingEscrow.claim, when there are unclaimed token available the recipient can choose which address to receive the token by beneficiary parameter, it means the token can still under recipient's possess. But in VestingEscrow.revokeAll, the unclaimed will be sent to the owner together with locked token, which means the unclaimed token will under owner's possess.

Impact

Some tokens should be sent to recipient but instead sent to owner

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L136-L144 https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L176-L189

Tool used

Manual Review

Recommendation

diff --git a/rio-vesting-escrow/src/VestingEscrow.sol b/rio-vesting-escrow/src/VestingEscrow.sol
index 1fdd103..80bdd2f 100644
--- a/rio-vesting-escrow/src/VestingEscrow.sol
+++ b/rio-vesting-escrow/src/VestingEscrow.sol
@@ -184,7 +184,8 @@ contract VestingEscrow is IVestingEscrow, Clone {
         isFullyRevoked = true;
         disabledAt = uint40(block.timestamp);

-        token().safeTransfer(_owner(), revokable);
+        token().safeTransfer(_owner(), locked());
+        token().safeTransfer(recipient(), unclaimed());
         emit VestingFullyRevoked(msg.sender, revokable);
     }

[nevillehuang]

Invalid, this is expected behavior, revoked tokens will be revoked back to owner given only owner can call this function. Additionally, this has the same exact design as the lido vesting escrow as seen here

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

pratraut commented:

'invalid as owner can revokeAll in the extreme situation where found recipient malicious so funds should go to owner only'