code-423n4 / 2022-04-backd-findings

6 stars 4 forks source link

Griefer can force withdrawal to treasury in `CvxCrvRewardsLocker#processExpiredLocks` #201

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/CvxCrvRewardsLocker.sol#L130-L145

Vulnerability details

Impact

The processExpiredLocks function in CvxCrvRewardsLocker is external and unpermissioned.

When locks expire, a griefer can call or frontrun calls to this function to force withdrawal to the treasury.

Proof of Concept

CvxCrvRewardsLocker#processExpiredLocks

    /**
     * @notice Processes expired locks.
     */
    function processExpiredLocks(bool relock) external override returns (bool) {
        if (relock) {
            require(!prepareWithdrawal, Error.PREPARED_WITHDRAWAL);
        }

        if (relock) {
            ICvxLocker(CVX_LOCKER).processExpiredLocks(relock);
        } else {
            ICvxLocker(CVX_LOCKER).withdrawExpiredLocksTo(treasury);
        }

        return true;
    }

Scenario:

Recommended Mitigation Steps

Consider adding the onlyGovernance modifier to this function, or using the value of preparedWithdrawal rather than the relock argument.

chase-manning commented 2 years ago

Could change to this maybe?

function processExpiredLocks(bool relock) external override returns (bool) {
    // if (relock) {
    //     require(!prepareWithdrawal, Error.PREPARED_WITHDRAWAL);
    // }

    if (!prepareWithdrawal) {
        ICvxLocker(CVX_LOCKER).processExpiredLocks(relock);
    } else {
        ICvxLocker(CVX_LOCKER).withdrawExpiredLocksTo(treasury);
    }

    return true;
}
chase-manning commented 2 years ago

This has no potential loss of funds so should be a low severity

gzeoneth commented 2 years ago

Downgrading to Low / QA

chase-manning commented 2 years ago

https://github.com/backdfund/protocol/pull/262