depositFor() in CrvDepositor, call arbitrary external contract which can be harmful for contract funds or user can lose funds incase of wrong address

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L168-L203

Vulnerability details

Impact

If user set wrong value for _stakeAddress his fund can be lost because contract don't check the value and transfer user minted tokens to that address.
attacker can set _stakeAddress and make contract call arbitrary contract's stakeFor() function. for example if Balancer had some stakeFor functionality, attacker can call that contract and make use of CrvDepositor balances in Balancer. CrvDepositor deposits tokens in crvBpt and attacker can make CrvDepsitor to call Balancer protocol contract's stakeFor() functions and mint himself some tokens in Balancer by CrvDepositor assets, as CrvDepositor calls Balancer so Balancer will allow that action. for this attack there should be stakeFor(to, amount) function in Balancer that uses crvBpt balance.

Proof of Concept

This is depositFor() code in CrvDepositor:

function depositFor(address to, uint256 _amount, bool _lock, address _stakeAddress) public {
        require(_amount > 0,"!>0");
        
        if(_lock){
            //lock immediately, transfer directly to staker to skip an erc20 transfer
            IERC20(crvBpt).safeTransferFrom(msg.sender, staker, _amount);
            _lockCurve();
            if(incentiveCrv > 0){
                //add the incentive tokens here so they can be staked together
                _amount = _amount.add(incentiveCrv);
                incentiveCrv = 0;
            }
        }else{
            //move tokens here
            IERC20(crvBpt).safeTransferFrom(msg.sender, address(this), _amount);
            //defer lock cost to another user
            uint256 callIncentive = _amount.mul(lockIncentive).div(FEE_DENOMINATOR);
            _amount = _amount.sub(callIncentive);

            //add to a pool for lock caller
            incentiveCrv = incentiveCrv.add(callIncentive);
        }

        bool depositOnly = _stakeAddress == address(0);
        if(depositOnly){
            //mint for to
            ITokenMinter(minter).mint(to,_amount);
        }else{
            //mint here 
            ITokenMinter(minter).mint(address(this),_amount);
            //stake for to
            IERC20(minter).safeApprove(_stakeAddress,0);
            IERC20(minter).safeApprove(_stakeAddress,_amount);
            IRewards(_stakeAddress).stakeFor(to,_amount);
        }
    }

As you can see there is no check for _stakeAddress, and in the end contract calls IRewards(_stakeAddress).stakeFor(to,_amount), and attacker can control all these variables.
CrvDepositor deposit user's tokens in crvBpt pool of Balancer. so if there were some function stackFor() in Balancer pool system, attacker can set that contract address as _stakeAddress, and CrvDepositor will call that function of that address and attacker can mint himself some other tokens and funds of CrvDepositor can be lost and he follow up calculations in CrvDepositor would be messed up too.
Of course if user set _stakeAddress wrong value, he can lost his deposits too.

Tools Used

VIM

Recommended Mitigation Steps

have some validity checks for _stakeAddress and make sure address registered in Aura systesm.

[dmvt]

Per #364 (comment) I have decided to downgrade this to QA.

[dmvt]

Grouping this with the warden’s QA report, #294