search

yaxis-project / metavault

The MetaVault will allow users to take advantage of the best yield farming strategies while minimizing gas fees and transferring difficult management decisions to an incentivized community governance.
https://yaxis.io/
MIT License
33 stars 19 forks source link

setRouter should also update allowances for other tokens #108

Open gpersoon opened 3 years ago

gpersoon commented 3 years ago

Based on HAECHI audit Axis v3/Smart Contract Security Analysis/Published on : Oct 13, 2021/Version v1.0, NativeStrategyCurve3Crv does not update Crvallowance setting a new router:

Vulnerability details

The Constructor of BaseStrategy.sol sets an allowance for ETH: https://github.com/yaxis-project/metavault/blob/aef0124094ed28945cde2e5b80f453cdbca1e8e5/contracts/v3/strategies/BaseStrategy.sol#L67

setRouter() updates this allowance: https://github.com/yaxis-project/metavault/blob/aef0124094ed28945cde2e5b80f453cdbca1e8e5/contracts/v3/strategies/BaseStrategy.sol#L103-L104

However the Strategy contracts also set allowances for _router, which are not updated with setRouter():

https://github.com/yaxis-project/metavault/blob/aef0124094ed28945cde2e5b80f453cdbca1e8e5/contracts/v3/strategies/NativeStrategyCurve3Crv.sol#L51

https://github.com/yaxis-project/metavault/blob/6c3eac8b45d639339373ea0d6321debf34d5c80c/contracts/v3/strategies/MIMConvexStrategy.sol#L92-L93

https://github.com/yaxis-project/metavault/blob/8f35c3da08c909c2e1879f20d733714ce4a76210/contracts/v3/strategies/GeneralConvexStrategy.sol#L78-L79

https://github.com/yaxis-project/metavault/blob/8f35c3da08c909c2e1879f20d733714ce4a76210/contracts/v3/strategies/ConvexStrategy.sol#L77-L78

Afterthought: reset the allowance for the previous router

Recommended mitigation steps

function setRouter(address _router) virtual external { require(msg.sender == manager.governance(), "!governance"); IERC20(weth).safeApprove(address(router), 0); // reset previous allowance router = ISwap(_router); IERC20(weth).safeApprove(address(_router), 0); IERC20(weth).safeApprove(address(_router), type(uint256).max); }

Implement an override function setRouter() in the following contracts:

For example: function setRouter(address _router) override external { require(msg.sender == manager.governance(), "!governance"); IERC20(_crv).safeApprove(address(router), 0); // reset previous allowance // except in NativeStrategyCurve3Crv.sol IERC20(_cvx).safeApprove(address(router), 0); // reset previous allowance super.setRouter(); IERC20(_crv).safeApprove(address(_router), 0); // check if necessary IERC20(_crv).safeApprove(address(_router), type(uint256).max); IERC20(_cvx).safeApprove(address(_router), 0); // check if necessary IERC20(_cvx).safeApprove(address(_router), type(uint256).max); // except in NativeStrategyCurve3Crv.sol }

Note: verify if setting the allowance to 0 first is important as is done in the current setRouter(); If it is important do it before the safeApprove, also in the constructors, to prevent a griefing attack For WETH is doesn't seem important.

uN2RVw5q commented 3 years ago

As far as I see, there are two approaches to solving this.

The first one is as above, by approving the corresponding tokens during setRouter.

The second approach is to call approveForSpender, before or after the setRouter call with the correct _spender and _amount.

https://github.com/yaxis-project/metavault/blob/832b83f999c3cd9effcff31bd9aba50a9d626b87/contracts/v3/strategies/BaseStrategy.sol#L80

I kinda favour the second approach, since it requires no changes in the codebase. The only issue is that this needs to go through governance. I assume this isn't a big deal as this can be done at the same time that setRouter governance proposal passes.

If the second approach is chosen, I recommend documenting this somewhere, that approveForSpender needs to be done alongside setRouter governance action.

gpersoon commented 3 years ago

The suggestion of @uN2RVw5q is possible as long as it is documented well, both at the source of setRouter() and also where government actions are documented.