Kicking is still possible and kick rewards accessible while in shutdown

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L386-L389

Vulnerability details

kickExpiredLocks is still allowed to be successfully run when locks[length - 1].unlockTime <= block.timestamp - rewardsDuration.mul(kickRewardEpochDelay) even when the system is in Shutdown.

This way once the system go to shutdown a malicious user can setup a bot that runs kickExpiredLocks immediately after the condition becomes true. This can be unexpected by lock owner as only funds withdrawals should be facilitated in the shutdown mode, while other mechanics are to be stopped.

Setting severity to medium as that's a divergence from expected behaviour of the system.

Proof of Concept

isShutdown part condition is meaningless when _checkDelay > 0:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L386-L389

        // e.g. now = 16
        // if contract is shutdown OR latest lock unlock time (e.g. 17) <= now - (1)
        // e.g. 17 <= (16 + 1)
        if (isShutdown || locks[length - 1].unlockTime <= expiryTime) {

As if locks[length - 1].unlockTime > expiryTime then the L402 will revert on subtraction as it will be currentEpoch < uint256(locks[length - 1].unlockTime):

uint256 epochsover = currentEpoch.sub(uint256(locks[length - 1].unlockTime)).div(rewardsDuration);

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L396-L405

            //check for kick reward
            //this wont have the exact reward rate that you would get if looped through
            //but this section is supposed to be for quick and easy low gas processing of all locks
            //we'll assume that if the reward was good enough someone would have processed at an earlier epoch
            if (_checkDelay > 0) {
                uint256 currentEpoch = block.timestamp.sub(_checkDelay).div(rewardsDuration).mul(rewardsDuration);
                uint256 epochsover = currentEpoch.sub(uint256(locks[length - 1].unlockTime)).div(rewardsDuration);
                uint256 rRate = AuraMath.min(kickRewardPerEpoch.mul(epochsover + 1), denominator);
                reward = uint256(locks[length - 1].amount).mul(rRate).div(denominator);
            }

I.e. locks[length - 1].unlockTime <= expiryTime is de facto required in all the cases and the system behavior for kicking doesn't change on shutdown, with the only difference that it will be low level error.

Recommended Mitigation Steps

Consider reverting kicks when the system is in shutdown:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L346-L349

    function kickExpiredLocks(address _account) external nonReentrant {
+       require(!isShutdown, "shutdown");
        //allow kick after grace period of 'kickRewardEpochDelay'
        _processExpiredLocks(_account, false, msg.sender, rewardsDuration.mul(kickRewardEpochDelay));
    }

[dmvt]

This is a commenting / documentation issue. The warden has failed to show how this would cause a loss of funds or generally cause issues with the operation of the protocol. The documentation should be updated to clearly describe the way this functions. Downgrading to QA

[dmvt]

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