Lines of code

Vulnerability details

Impact

Balance functions within YVaultLPFarming, YVault and Controller rely on related contracts to calculate and feedback their supposedly held balances. The relation of those contracts will be illustrated in the POC section, but TLDR of the problem is that none of the upstream contracts check whether a contract requesting balance calculation is benign. This allows attackers to deploy impersonating contracts that have "false balances" displayed.

Combined with the lack of check in deposit related functions, users can be easily misguided by impersonating contracts instead of benign one. Any deposit into impersonating contracts can then be automatically claimed by the original benign instance, thus leading to monetary loss of users.

The key point of this attack is lack of checks makes it hard to spot fake contracts. The only difference between a real contract and a fake one would be its deployer, and it is not reasonable to expect everyday users to be tech savvy enough to examine deployment transactions just to check if a contract is legal.

Thus we conclude it is highly possible that attackers can adopt this contract impersonating technique to trick users into depositing tokens into evil contracts. Attackers can then profit by simply using the benign contract.

Proof of Concept

Balance calculation can be summarized into the diagram shown below

        YVaultLPFarming --request-> YVault --request-> Controller --request-> StrategyPUSDConvex
                        <-response-        <-response-            <-response-

Theoretically, when handling a balance calculation request from a lower level in the hierarchy, handlers should check the identity of requester, and proceed to respond to the balance of(and only of) the requester.

However, this check is not present in the JPEGED system.

We examine the interaction between YVaultLPFarming and YVault to get an overview of the systematic flaw.

YVaultLPFarming calculates JPEG balance in _computeUpdate by adding vault.balanceOfJPEG() and jpeg.balanceOf(address(this)), and YVault forwards the balance calculation to Controller without ever checking address of YVaultLPFarming

contract YVaultLPFarming {
    ...
    function _computeUpdate() internal view returns (uint256 newAccRewardsPerShare, uint256 currentBalance) {
        currentBalance = vault.balanceOfJPEG() + jpeg.balanceOf(address(this));
        ...
    }
    ...
}

contract YVault{
    ...
    function balanceOfJPEG() external view returns (uint256) {
        return controller.balanceOfJPEG(address(token));
    }
    ...
}

This creates the problem where when a malicious YVaultLPFarming has a vault pointed to the same YVault as some benign YVaultLPFarming, it will have incorrect estimation of currentBalance. In fact, since JPEG are never intended to stay in YVaultLPFarming for a long time, the currentBalance will most likely be synchronized to the benign YVaultLPFarming.

Incorrect display of balance in a fake YVaultLPFarming itself is already a problem, given that the malicious contract can be set up with storage exactly the same as a benign contract. The only difference will be transaction history, an aspect most users won't dig too much into. And users can easily be misled into believing that the fake contract is actually legitimate.

Now we carry on to see how tricking users into using fake contracts can further benefit attackers.

It turns out that the lack of checks does not stop at balance calculation functions, all deposit related functions also lack checks. While the YVaultLPFarming and YVault pair is still susceptible to attack, we shift our focus to the slightly more complex pair of YVault and Controller to demonstrate the problem more clearly.

YVault.earn is called to transfer CRV token to Controller for yield, and Controller.earn will be called in YVault.earn to handle the part of sending CRV tokens into strategy. Once again, we can see that while Controller has the ability to check whether msg.sender is a benign vault through vaults[_token], it chooses not to do so.

contract YVault {
    ...
    function earn() external {
        uint256 _bal = available();
        token.safeTransfer(address(controller), _bal);
        controller.earn(address(token), _bal);
    }
    ...
}

contract Controller {
    ...
    function earn(IERC20 _token, uint256 _amount) external {
        IStrategy strategy = strategies[_token];
        _token.safeTransfer(address(strategy), _amount);
        strategy.deposit();
    }
    ...
}

Combined with the previous incorrect balance displacement, we can see that attackers can likely trick users into trusting fake contract instances and deposit their tokens from those.

Finally, let's see why deposit of tokens in fake contracts will lead to monetary loss. We focus on the YVault and Controller pair again.

While balance calculation and token deposit does not check the identity of the requester, withdraw does. The YVault.withdraw function attempts to full tokens from YVault when required by calling Controller.withdraw.

However, since Controller.withdraw checks the requester of withdraw against vaults[_token], thus the call will always revert for fake YVault.

contract YVault {
    ...
    function withdraw(uint256 _shares) public noContract(msg.sender) {
        ...
        if (vaultBalance < backingTokens) {
            uint256 toWithdraw = backingTokens - vaultBalance;
            controller.withdraw(address(token), toWithdraw);
        }
        ...
    }
    ...
}

contract Controller {
    ...
    function withdraw(IERC20 _token, uint256 _amount) public {
        require(msg.sender == vaults[_token], "NOT_VAULT");
        strategies[_token].withdraw(_amount);
    }
    ...
}

This causes any tokens deposited through fake contracts to become un-withdrawable, and since fake instances and benign instances share the same upstream contract, the depossited tokens will actually be assigned to the benign contract.

Thus if an attacker managed to trick users to use malicious contracts(which we have argued earlier to be highly probable), he can enjoy the profit by simply using benign contracts and collecting victim deposited tokens.

While we only illustrated the concept for specific pairs, any two linked components in the relation chain shown above suffers from this problem and can be targeted by attackers.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Check before balance calculation and depositing. This can drastically lower the authenticness of fake contracts and make it unlikely for users to be tricked by attackers.

An example fix of the YVault to Controller contract is shown below

    function balance() public view returns (uint256) {
        uint256 tokenBalance = token.balanceOf(address(this));
        if(controller.vaults[token] == address(this))
            tokenBalance += controller.balanceOf(address(token));
        return tokenBalance;
    }

    // @return The amount of JPEG tokens claimable by {YVaultLPFarming}
    function balanceOfJPEG() external view returns (uint256) {
        if(controller.vaults[token] != address(this))
            return 0;
        return controller.balanceOfJPEG(address(token));
    }

    function earn() external {
        require(controller.vaults[token] == address(this), "Invalid Vault-Controller pair");
        uint256 _bal = available();
        token.safeTransfer(address(controller), _bal);
        controller.earn(address(token), _bal);
    }

code-423n4 / 2022-04-jpegd-findings