Breeje - Recipient can `claim` all vested token instantly if token used is Proxied Token

[sherlock-admin2]

Breeje

medium

Recipient can claim all vested token instantly if token used is Proxied Token

Summary

Certain tokens like SNX, sETH, and sBTC have multiple entry points due to their proxy contract structure. The recoverERC20 function in the VestingEscrow contract does not appropriately handle scenarios involving Proxied Tokens, allowing a recipient to instantly claim all vested tokens without adhering to the vesting period.

Vulnerability Detail

Background:

As per Readme:

Which ERC20 tokens do you expect will interact with the smart contracts? Any, with the exception of rebasing tokens and tokens that charge a fee on transfer.

As it is explicitly mentioned that all types of ERC20 tokens are in scope except those with transfer fees or rebasing mechanisms, Reporting this one.

Some ERC20 tokens use a proxy contract, resulting in at least two entry points (proxy and implementation) for their functionality.

Such token type is listed here in Famous Weird Token Repo.

An example is Synthetix’s ProxyERC20 token contract, including tokens like SNX, sUSD, and sBTC. These tokens can be interacted through multiple entry points.

Resource: Synthetix ProxyERC20 Docs

Have a look at how ProxyERC20 is implemented:

File: ProxyERC20.sol

pragma solidity ^0.5.16;

// Inheritance
import "./Proxy.sol";
import "./interfaces/IERC20.sol";

// https://docs.synthetix.io/contracts/source/contracts/proxyerc20
contract ProxyERC20 is Proxy, IERC20 {
    constructor(address _owner) public Proxy(_owner) {}

    // ------------- ERC20 Details ------------- //

    function name() public view returns (string memory) {
        // Immutable static call from target contract
        return IERC20(address(target)).name();
    }

    function symbol() public view returns (string memory) {
        // Immutable static call from target contract
        return IERC20(address(target)).symbol();
    }

    function decimals() public view returns (uint8) {
        // Immutable static call from target contract
        return IERC20(address(target)).decimals();
    }

    // ------------- ERC20 Interface ------------- //

    /**
     * @dev Total number of tokens in existence
     */
    function totalSupply() public view returns (uint256) {
        // Immutable static call from target contract
        return IERC20(address(target)).totalSupply();
    }

    /**
     * @dev Gets the balance of the specified address.
     * @param account The address to query the balance of.
     * @return An uint256 representing the amount owned by the passed address.
     */
    function balanceOf(address account) public view returns (uint256) {
        // Immutable static call from target contract
        return IERC20(address(target)).balanceOf(account);
    }

    /**
     * @dev Function to check the amount of tokens that an owner allowed to a spender.
     * @param owner address The address which owns the funds.
     * @param spender address The address which will spend the funds.
     * @return A uint256 specifying the amount of tokens still available for the spender.
     */
    function allowance(address owner, address spender) public view returns (uint256) {
        // Immutable static call from target contract
        return IERC20(address(target)).allowance(owner, spender);
    }

    /**
     * @dev Transfer token for a specified address
     * @param to The address to transfer to.
     * @param value The amount to be transferred.
     */
    function transfer(address to, uint256 value) public returns (bool) {
        // Mutable state call requires the proxy to tell the target who the msg.sender is.
        target.setMessageSender(msg.sender);

        // Forward the ERC20 call to the target contract
        IERC20(address(target)).transfer(to, value);

        // Event emitting will occur via Synthetix.Proxy._emit()
        return true;
    }

    /**
     * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
     * Beware that changing an allowance with this method brings the risk that someone may use both the old
     * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
     * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
     * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
     * @param spender The address which will spend the funds.
     * @param value The amount of tokens to be spent.
     */
    function approve(address spender, uint256 value) public returns (bool) {
        // Mutable state call requires the proxy to tell the target who the msg.sender is.
        target.setMessageSender(msg.sender);

        // Forward the ERC20 call to the target contract
        IERC20(address(target)).approve(spender, value);

        // Event emitting will occur via Synthetix.Proxy._emit()
        return true;
    }

    /**
     * @dev Transfer tokens from one address to another
     * @param from address The address which you want to send tokens from
     * @param to address The address which you want to transfer to
     * @param value uint256 the amount of tokens to be transferred
     */
    function transferFrom(
        address from,
        address to,
        uint256 value
    ) public returns (bool) {
        // Mutable state call requires the proxy to tell the target who the msg.sender is.
        target.setMessageSender(msg.sender);

        // Forward the ERC20 call to the target contract
        IERC20(address(target)).transferFrom(from, to, value);

        // Event emitting will occur via Synthetix.Proxy._emit()
        return true;
    }
}

Link to Code

As you can see, this proxy contract (ProxyERC20) utilizes a target where the main implementation resides. In the target, validation is implemented to ensure that the call originates either from the Proxy or the direct implementation, providing flexibility for user interactions with anyone.

Just to illustrate, here are some examples of tokens which can be affected by this vulnerability shown next:

1. SNX Token: Link | Current Market Cap: 1.2 Billion $
2. sUSD Token: Link | Current Market Cap: 56 Million $
3. sETH Token: Link | Current Market Cap: 38 Million $
4. sBTC Token: Link | Current Market Cap: 13 Million $

Market Cap Data Source: Etherscan

Issue

The VestingEscrow contract is designed to hold tokens until the vesting period elapses, allowing users to claim unvested tokens over time.

However, the recoverERC20 function, intended to transfer an entire amount if token_ is not equal to token(), fails to account for Proxied Tokens.

File: VestingEscrow.sol

    function recoverERC20(address token_, uint256 amount) external {
        uint256 recoverable = amount;
        if (token_ == address(token())) {
            uint256 available = token().balanceOf(address(this)) - (locked() + unclaimed());
            recoverable = Math.min(recoverable, available);
        }
        if (recoverable > 0) {
@->         IERC20(token_).safeTransfer(recipient(), recoverable);
            emit ERC20Recovered(token_, recoverable);
        }
    }

Link to Code

In the case of using Proxied Tokens, the recoverERC20 function acts as a backdoor for the recipient to instantly claim all tokens by utilizing the token address of the proxy contract. The crucial token_ == address(token()) check is bypassed in such scenarios, enabling the user to successfully claim the entire vested amount without waiting for the intended vesting period.

Impact

Recipients can exploit the vulnerability to claim tokens instantly, bypassing the vesting period.

Code Snippet

Shown Above.

Tool used

Manual Review

Recommendation

To address this vulnerability, a require check should be added at the end in recoverERC20 function:

  function recoverERC20(address token_, uint256 amount) external {
        uint256 recoverable = amount;
        if (token_ == address(token())) {
            uint256 available = token().balanceOf(address(this)) - (locked() + unclaimed());
            recoverable = Math.min(recoverable, available);
        }
        if (recoverable > 0) {
            IERC20(token_).safeTransfer(recipient(), recoverable);
            emit ERC20Recovered(token_, recoverable);
        }
+       require(token().balanceOf(address(this)) >= locked() + unclaimed());
    }

Duplicate of #62

[sherlock-admin2]

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

_rahul commented:

Invalid: Non standard / weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.

Shaheen commented:

Valid Medium, nice finding

0xLogos commented:

technically valid medium according to contest readme, but imho it's too much unrealistic for someone to vest synthetix's tokens

pratraut commented:

'valid as token with two addresses can steal tokens from escrow contract'