code423n4 commented 1 year ago

Lines of code

Vulnerability details

Impact

Unintended funcionality, users have to pay for the gas fee of vault deposit as well on every deposit

Description

Users can deposit to a yieldbox strategy via YieldBox - depositAsset(). depositAsset() calls deposited on the strategy contract asset.strategy.deposited(amount). Every yieldbox strategy contract inherits BaseERC20Strategy and overrides it's _deposited function to check if depositThreshold have been met whenever a deposit have been made. If it is met: the function deposits to the corresponding vault / pool. If is not met it stays queued until queued > depositThreshold than it is deposited.

YearnStrategy.sol - deposited()

    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            vault.deposit(queued, address(this));
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

However users can deposit before a depositThreshold have been set via setDepositThreshold() so in the above function depositThreshold would equal 0 and queued amount will always be greater and will be deposited. Even when the depositThreshold is set, the transaction can be front run. Every time someone deposits the vault.deposit() fee also have to be paid by the user depositing which is not cost effective.

All yieldbox strategy contracts are vulnerable since they share similar _deposited() logic:

if (queued > depositThreshold) {
    pool.deposit(queued);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider setting the depositThreshold in the constructor of the strategy contracts. Alternatively don't allow any deposits in _deposited() if the depositThreshold have not yet been set: if (depositThreshold != 0 && queued > depositThreshold)

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

QA

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)