Discovery of a Security Vulnerability in a Yearn Vault

Many people in the Ethereum community know Yearn as one of the most famous DeFi platforms. I’ve been working with @matkam, a fellow Yearn user I met in the Discord, quite a bit over the past few days who’s been a super helpful, curious, and fun person to chat while we both share our learnings on Yearn vaults and the surrounding ecosystem. He was kind enough to leak some alpha and convinced me to enter into the brand new crvGUSD vault vault. He even pretty quickly diagnosed an issue with the vault strategy that would prevent it from having it’s first harvest (the mechanism for vaults to earn).

Discovering the vulnerability itself

While looking into Matt’s discovery myself, I began writing some scripts to determine my earnings upon harvest and compare those earnings to other vaults I could be in. I decided to take it a step further and wrote one script to determine which vaults had the nearest upcoming harvest. With this script I noticed something a little bit bizarre:

[{ vaultAlias: 'crvGUSD',
    vaultAddress: '0xcC7E70A958917cCe67B4B87a8C30E6297451aE98',   
    strategyAddress: '0xD42eC70A590C6bc11e9995314fdbA45B4f74FABb',
    requiredHarvest: 5000,
    currentHarvestableCrv: 6577.878622730519,
    remainingUntilHarvest: 0,
    workable: true },

  { vaultAlias: 'GUSD',
    vaultAddress: '0xec0d8D3ED5477106c6D4ea27D90a60e594693C90',   
    strategyAddress: '0xc8327D8E1094a94466e05a2CC1f10fA70a1dF119',
    requiredHarvest: 10000,
    currentHarvestableCrv: 6577.878622730519,
    remainingUntilHarvest: 3422.121377269481,
    workable: false },
    ...
]

The snippet above shows data for just two (of many) of the vaults returend by my script. But you might notice that the currentHarvestableCrv values are the exact same for both the new crvGUSD vault and the old GUSD. Very strange - how could this be? Upon further investigation, it turns out they were sharing the same strategy proxy address and both strats were reporting balances based on that. Of course these two strategies should be totally distinct from one another, not using a shared balance. The consequences of this are that a harvest() call on one strategy will steal all the funds from the other strategy.

I brought this to @matkam and we both had the same thought: we need to get this fixed before the GUSD vault is able to make its next harvest, in which case it would steal the funds from our crvGUSD vault. Since the GUSD vault harvest threshold was 10,000 I determined we had a couple days worth of time before this might happen. The first thing I checked was that the GUSD strategy contract was not assigned to the Keeper. It was not, and this is good, because it would prevent an honest keeper from automatically calling harvest as soon as the crv balance hit 10,000.

But, would this prevent any other person from calling it? In most newer strategy contracts, the harvest call is protected by this following line of code:

require(msg.sender == keeper || msg.sender == strategist || msg.sender == governance, "!keepers");

However, the the old GUSD vault is different (I suspect it’s because it was created before explicitly naming a keeper address in the contract became common convention). It’s access control on the harvest function looks like this:

require(msg.sender == strategist || msg.sender == governance || msg.sender == tx.origin, "!authorized");

The key thing to notice here is the third condition which permits a harvest call whenever msg.sender == tx.origin evaluates to true. According to the Solidity docs, tx.origin evaluates to the original sender of a transaction in a call chain (excludes contract addresses). To bring this back to our scenario, this means that any user account could successfully authenticate to make a harvest() call. Not good!

Potential Impacts of this Vulnerability

Any Ethereum account could harvest rewards to the wrong strategy (GUSD - StrategyCurveGUSDProxy) and steal them from the correct strategy (crvGUSD - StrategyCurveGUSDVoterProxy). I immediately reported it to some of the Yearn developers on Discord who confirmed that this is a problem (thanks @luciano, @Macarse, @orb_ball).

@Banteg appears to have taken it from there and put together an extremely well thought out write-up of the situation and the downstream impacts. Included in this, he discovered that the price per full share (ppfs) of the GUSD vault was inaccurate. One user was able to drain the entire vault (making multiple times his original investment), while other users were left with nothing. He also produced a full list of users affected and outlines some ideas for reimbursement to those users.