Unauthorized Token Claim in VestingEscrow.sol

Summary

In VestingEscrow.sol anyone can claim and not just the recepient who was designed to do so.

Vulnerability Detail

Although the claim(address beneficiary, uint256 amount) function in VestingEscrow.sol is protected by the onlyRecipient modifier, it can still be bypassed within the VestingEscrow contract context. This vulnerability allows any user to withdraw the entire amount deposited during the contract's creation.

The POC bellow showcase exactly how a normal user can claim all the tokens, you can add this function in VestingEscrow.t.sol .

    function testAnyoneCanClaimAndNotJustRecepient() public {
        vm.warp(endTime); // Sets block.timestamp to endTime

        vm.prank(USER); // Here you can see that we are conected as USER and not  recipient

        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(USER)); //we call delegate from user account

        console.log("Balance of user before the claim",token.balanceOf(USER));   // this will be 0 before claim

        deployedVesting.claim(USER, type(uint256).max); //we call claim with max value and the user address as beneficiary 

        console.log("Balance of user after the claim",token.balanceOf(USER));  // this will be 1 ETH since at setUp() we specified the amount to be 1 ETH

    }

To clarify, we ensured that we reach the endTime to be eligible for token claiming. Just before initiating the claim process, we execute the delegate function with the USER address.

Impact

The vulnerability allows anyone to claim and collect funds from the contract that were originally deposited during its creation.

Code Snippet

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

Tool used

Manual Review

Recommendation

I highly recommend the project to replace the usage of onlyRecipient as a safeguard with onlyOwner which represent owner of that contract.

nevillehuang commented 9 months ago

Invalid, both delegate() and claim() can only be called by the recipient as indicated by the onlyRecipient modifier

sherlock-admin2 commented 9 months ago

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

pratraut commented:

'invalid as testcase is failing when making vm.prank to vm.startPrank. Due to just prank its using USER only for inner function call and not for outer delegate call'

sherlock-audit / 2024-01-rio-vesting-escrow-judging

0xhashiman - Unauthorized Token Claim in VestingEscrow.sol #39