recursiveEth - Fee-on-transfer tokens aren't supported

[sherlock-admin2]

recursiveEth

high

Fee-on-transfer tokens aren't supported

Summary

Fee-on-transfer tokens aren't supported by the current escrow implementation

Vulnerability Detail

In VestingEscrowFactory.sol the amount varaible is used to transfer the tokens to address where Escrow contract is deployed. https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L65

escrow = vestingEscrowImpl.clone(
  abi.encodePacked(
    address(this),
    token,
    recipient,
    vestingStart,
    vestingStart + vestingDuration,
    cliffLength,
    amount
  )
);

but the actual token transfer to the Escrow will be less than the Amount which they are keeping track of , VestingEScorw:totalLocked() it represent the amount of tokens locked inside the contract which was assigned the value of amount in vestingEScorwFactory.sol at the time of deployement. This will cause the call to create a new instance of Escrow trigger the following revert in VestingEscrow:intialize().

  if (_token.balanceOf(address(this)) < _totalLocked) revert INSUFFICIENT_BALANCE();

suppose totalLocked value is 5000, but actual token value locked inside is 4900 this if statment cause revert because totalLocked balance will be always greater.

Impact

The protocol prevents the use of fee-on-transfer tokens without explicitly defining these conditions. The inability to do so doesn't require malicious action by either party and given the sponsor comment with regard to compatible tokens this doesn't appear to be addressed:

But, the organization could do whatever they want - just we would recommend against that."

Code Snippet

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

 function initialize(bool _isFullyRevokable, bytes calldata _initialDelegateParams) external {
        address _factory = address(factory());
        uint256 _totalLocked = totalLocked();
        uint40 _endTime = endTime();
        IERC20 _token = token();

        if (msg.sender != _factory) revert NOT_FACTORY(msg.sender);
        // @audit because totalLocked is set by amount value , which is not real value which will be present in contract after transfer
        if (_token.balanceOf(address(this)) < _totalLocked) revert INSUFFICIENT_BALANCE();
        disabledAt = _endTime;
        isFullyRevokable = _isFullyRevokable;
        if (_initialDelegateParams.length != 0) _delegate(_initialDelegateParams);

        emit VestingEscrowInitialized(
            _factory,
            recipient(),
            address(_token),
            _totalLocked,
            startTime(),
            _endTime,
            cliffLength(),
            _isFullyRevokable
        );
    }

Tool used

Manual Review

Recommendation

Change the value passed to totalLocked while deploying VestingEscrowFactory from amount to the actual amount to tokens transferred to the Escrow, this is just a thought their may be some other ways.

[nevillehuang]

Invalid, as mentioned in contest details, FOT tokens are not supported

Are there any FEE-ON-TRANSFER tokens interacting with the smart contracts?

No

[sherlock-admin2]

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

pratraut commented:

'invalid as fee on transfer token is not in scope'