search

yearn / yearn-protocol

Yearn smart contracts
https://yearn.finance
GNU Affero General Public License v3.0
441 stars 212 forks source link

fix: check for failure of proxy execute #19

Closed lbertenasco closed 3 years ago

lbertenasco commented 3 years ago

https://github.com/iearn-finance/yearn-protocol/pull/19#discussion_r500612532 I'm not sure if it works since the issue was on mainnet on a specific keeper configuration.

banteg commented 3 years ago

I've updated the exploit test to use Uniswap v2 WETH/DAI pool as a hacker, since it has the highest DAI balance. In such configuration is was able to drain the vault in 13 iterations:

1  vault: 204,794,149.30 yCRV (+0.00)
1 hacker: 174,385,544.27 yCRV (+0.00)
2  vault: 199,986,681.61 yCRV (-4,807,467.69)
2 hacker: 178,297,046.90 yCRV (+3,911,502.63)
3  vault: 180,974,697.88 yCRV (-23,819,451.42)
3 hacker: 196,322,485.48 yCRV (+21,936,941.21)
4  vault: 158,594,370.28 yCRV (-46,199,779.03)
4 hacker: 217,609,299.02 yCRV (+43,223,754.75)
5  vault: 132,336,285.05 yCRV (-72,457,864.25)
5 hacker: 242,648,047.32 yCRV (+68,262,503.05)
6  vault: 102,062,444.74 yCRV (-102,731,704.56)
6 hacker: 271,557,278.20 yCRV (+97,171,733.93)
7  vault: 68,799,926.99 yCRV (-135,994,222.31)
7 hacker: 303,295,696.97 yCRV (+128,910,152.70)
8  vault: 36,242,577.46 yCRV (-168,551,571.84)
8 hacker: 334,173,781.26 yCRV (+159,788,236.99)
9  vault: 11,768,677.47 yCRV (-193,025,471.84)
9 hacker: 356,854,442.85 yCRV (+182,468,898.58)
10  vault: 1,411,352.82 yCRV (-203,382,796.48)
10 hacker: 365,375,708.66 yCRV (+190,990,164.39)
11  vault: 21,556.65 yCRV (-204,772,592.65)
11 hacker: 364,931,677.30 yCRV (+190,546,133.03)
12  vault: 5.09 yCRV (-204,794,144.21)
12 hacker: 363,128,462.72 yCRV (+188,742,918.45)
13  vault: 0.00 yCRV (-204,794,149.30)
13 hacker: 361,312,825.47 yCRV (+186,927,281.20)
hacker has earned: +186,927,281.20

I think the test is robust enough to test the fix. I also changed to one iteration since any increase in hacker's balance should be considered a failure.

banteg commented 3 years ago

The test has failed as expected with hacker making it with 3.9 million, we can proceed with a fix that passes it.

>       assert ycrv.balanceOf(hacker) <= before
E       AssertionError: assert 178645048179063124162467674 <= 174735187241305314522350570
E        +  where 178645048179063124162467674 = <ContractCall 'balanceOf(address account)'>(<Account '0xA478c2975Ab1Ea89e8196811F51A7B7Ade33eB11'>)
banteg commented 3 years ago

To prevent Evil Gauge attack in the future, besides access control in StrategyProxy, we better include a StrategyProxy.lock() as a part of harvest() routine in all the strategies that use it.