sherlock-audit / 2022-10-rage-trade-judging

1 stars 0 forks source link

simon135 - An attacker can give weth to the contract and can send a little weth and cause the else statement to get caused increasing the seniorVaultWethRewards or deceasing and making it zero #50

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

simon135

high

An attacker can give weth to the contract and can send a little weth and cause the else statement to get caused increasing the seniorVaultWethRewards or deceasing and making it zero

Summary

An attacker can send weth to the contract that can be small enough for the function to revert or be too much and not go into the right-if statement causing a loss of funds

Vulnerability Detail

2 scenarios: 1. since the attacker transferred weth to the contract the harvest won't happen and the attacker can do this at every harvest call and make sure nobody gets the harvest. or not hit the first if statement which balances glp through the batch contract which doesn't happen to cause glp to be not balanced causing liquidations to happen because glp is not balanced and if other tokens aren't doing well. Also, the assumption is broken because it's not evenly balanced.

  1. There is not enough weth and if the protocol fee is more than the weth in the contract it will revert. So on the first few harvests, the function will revert which can cause the holders of the strategy not to get the harvest amount which then there Are no incentives to keep funds in.

    Impact

    loss of funds and loss of incentives for users

    Code Snippet

    
        // total weth harvested which is not compounded
        // its possible that this is accumulated value over multiple rebalance if in all of those it was below threshold
        uint256 wethHarvested = state.weth.balanceOf(address(this)) - state.protocolFee - state.seniorVaultWethRewards;
    
        if (wethHarvested > state.wethConversionThreshold) {
            // weth harvested > conversion threshold
            uint256 protocolFeeHarvested = (wethHarvested * state.feeBps) / MAX_BPS;
            // protocol fee incremented
            state.protocolFee += protocolFeeHarvested;
    
            // protocol fee to be kept in weth
            // remaining amount needs to be compounded
            uint256 wethToCompound = wethHarvested - protocolFeeHarvested;
    
            // share of the wethToCompound that belongs to senior tranche
            uint256 dnGmxSeniorVaultWethShare = state.dnGmxSeniorVault.getEthRewardsSplitRate().mulDivDown(
                wethToCompound,
                FeeSplitStrategy.RATE_PRECISION
            );
            // share of the wethToCompound that belongs to junior tranche
            uint256 dnGmxWethShare = wethToCompound - dnGmxSeniorVaultWethShare;
    
            // total senior tranche weth which is not compounded
            uint256 _seniorVaultWethRewards = state.seniorVaultWethRewards + dnGmxSeniorVaultWethShare;
    
            uint256 glpReceived;
            {
                // converts junior tranche share of weth into glp using batching manager
                // we need to use batching manager since there is a cooldown period on sGLP
                // if deposited directly for next 15mins withdrawals would fail
                uint256 price = state.gmxVault.getMinPrice(address(state.weth));
    
                uint256 usdgAmount = dnGmxWethShare.mulDivDown(
                    price * (MAX_BPS - state.slippageThresholdGmxBps),
                    PRICE_PRECISION * MAX_BPS
                );

https://github.com/sherlock-audit/2022-10-rage-trade/blob/main/dn-gmx-vaults/contracts/libraries/DnGmxJuniorVaultManager.sol#L188
## Tool used

Manual Review

## Recommendation
send a little weth to the contract or find  to only allow harvest after a certain time and  put access control on the function
0xDosa commented 1 year ago

Could you explain the issue better? A few questions:

Evert0x commented 1 year ago

Planning to close this issue and leave room for the submitter to provide more context in the post announcement escalation process.