yAxis contest - 0xsanson's results

Handle

0xsanson

Vulnerability details

Impact

NativeStrategyCurve3Crv._harvest calls getMostPremium to get the best stablecoin to convert to. This function however is wrong in the case of balancesUSDC = balancesUSDT < balancesDAI, because it returns DAI, when it should be USDC or USDT. This is naturally a rare occasion, but a bad actor can set the balances (by depositing/withdrawing the Curve pool) like this just before the harvest function is called. Since this would imbalance even more the pool, the bad actor could also gain a profit by making the right swaps after the harvest.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/strategies/NativeStrategyCurve3Crv.sol#L83

Tools Used

editor

Recommended Mitigation Steps

Convert all < into <= inside getMostPremium().

Handle

0xsanson

Vulnerability details

Impact

In BaseStrategy.approveForSpender there's a _token.safeApprove(_spender, _amount) call. This call will revert if the allowance is already different from zero.

Proof of Concept

Openzeppelin's SafeERC20: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L53

Tools Used

editor

Recommended Mitigation Steps

Add a _token.safeApprove(_spender, 0) first.

Handle

0xsanson

Vulnerability details

Impact

When adding a strategy in Controller.sol, a variable cap is passed to _vaultDetails[_vault].caps[_strategy]. I guess this is the maximum balance allowed for this strategy, but this is actually never used in the current implementation.

Tools Used

editor

Recommended Mitigation Steps

Check if this is a wanted feature of the protocol, and (if positive) add some checks to enforce it.

Handle

0xsanson

Vulnerability details

Impact

In Controller.reorderStrategies there's no check that _strategy1 and 2 are strategies of _vault. If the strategist by mistake calls this function using a different strategy, index evaluates as zero, effectively replacing the first vault's strategy with a non-appropriate one.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/controllers/Controller.sol#L215

Tools Used

editor

Recommended Mitigation Steps

Consider adding require(_vaultStrategies[_strategy1] == _vault) (idem for _strategy2). These checks can also replace require(manager.allowedStrategies(_strategy1/2), "!_strategy1/2").

Handle

0xsanson

Vulnerability details

Impact

In Vault.sol, the state variable totalDepositCap is 'the maximum amount of value that can be deposited to the metavault at a time' according to the comment at L97. The way it's used instead suggests a different meaning, i.e. 'the maximum amount of value that can be deposited to the metavault in total' (see L200).

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/Vault.sol#L97 https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/Vault.sol#L97

Tools Used

editor

Recommended Mitigation Steps

Check what's the intended behavior and either change the documentation or the usage in the deposit function.

Handle

0xsanson

Vulnerability details

Impact

In Controller.sol it's possible to change maxStrategies to a different value. There's no check however that the new value is greater than the present number of strategies.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/controllers/Controller.sol#L316

Tools Used

editor

Recommended Mitigation Steps

It's possible that this is an intended behaviour. If not, add a check for _maxStrategies >= _vaultDetails[_vault].strategies.length.

Handle

0xsanson

Vulnerability details

Impact

Functions Earn and Harvest of Harvester.sol work also when the manager is set to halted. Probably this is an unintented behaviour.

Tools Used

editor

Recommended Mitigation Steps

Add notHalted modifier to aforementioned functions.

Handle

0xsanson

Vulnerability details

Impact

In StablesConverter.convert there are multiple storage reads of tokens[i] that add up gas. Consider saving the variable in memory.

Tools Used

editor

Recommended Mitigation Steps

Rewrite

IERC20 _token;    // add this
for (uint8 i = 0; i < 3; i++) {
	_token = tokens[i];  // add this
    //if (_output == address(tokens[i])) {
    if (_output == address(_token)) {
        //uint256 _before = tokens[i].balanceOf(address(this));
        uint256 _before = _token.balanceOf(address(this));
        stableSwap3Pool.remove_liquidity_one_coin(
            _inputAmount,
            i,
            _estimatedOutput
        );
        //uint256 _after = tokens[i].balanceOf(address(this));
        uint256 _after = _token.balanceOf(address(this));
        _outputAmount = _after.sub(_before);
        //tokens[i].safeTransfer(msg.sender, _outputAmount);
        _token.safeTransfer(msg.sender, _outputAmount);
        return _outputAmount;
    }
}

Handle

0xsanson

Vulnerability details

Impact

The harvester can initiate the earn process by calling the Vault.earn. At the end of this function an Earn(_token, _balance) event is emitted. Before this, the execution is passed to the Controller.earn function, which emits another event Earn(_token, _strategy). Since these two events are emitted always together (Controller.earn can be called only inside Vault.earn), it's more efficient to emit a single event Earn(_token, _strategy, _amount) at the end of Controller.earn. This should gain a little gas (one less indexed data) and it's less confusing.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/controllers/Controller.sol#L437 https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/Vault.sol#L157

Tools Used

editor

Handle

0xsanson

Vulnerability details

Impact

During the _harvest function in NativeStrategyCurve3Crv.sol, there's a call to _deposit() only if (balanceOfWant() > 0). This if-statement can be removed since _deposit calculates again balanceOfWant() and makes the same check. This way the function saves a .balanceOf call.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/strategies/NativeStrategyCurve3Crv.sol#L123

Tools Used

editor

Handle

0xsanson

Vulnerability details

Impact

In Harvester.sol, harvestNextStrategy harvests the first strategy then shifts the array such that the second strategy becomes the first (so on future calls, all the strategy get harvested sequentially). A more efficient way (in terms of gas) to do this would be to have a state variable uint256 lastStrategyHarvest that starts at 0 and acts like an index for the last used strategy.

Tools Used

editor

Recommended Mitigation Steps

harvestNextStrategy would be rewritten like the following. Also removeStrategy may need some modifications depending on index.

function harvestNextStrategy(
    address _vault,
    uint256 _estimatedWETH,
    uint256 _estimatedYAXIS
)
    external
{
    require(canHarvest(_vault), "!canHarvest");
    uint256 k = strategies[_vault].addresses.length;    
    if (lastStrategyHarvest >= k) {
    	lastStrategyHarvest = 0;
    }
    address strategy = strategies[_vault].addresses[lastStrategyHarvest];
    harvest(controller, strategy, _estimatedWETH, _estimatedYAXIS);
    lastStrategyHarvest = lastStrategyHarvest + 1;
    // solhint-disable-next-line not-rely-on-time
    strategies[_vault].lastCalled = block.timestamp;
}