code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L375-L377 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L142

Vulnerability details

Impact

A user has the possibility to "rage-quit" staked tokens without having to wait for them to be fully vested (penalties apply).

But a (malicious) owner of the InfinityStaker contract is able to temporarily "pause" the rage quit functionality InfinityStaker.rageQuit() by changing the storage variable INFINITY_TREASURY to the zero-address. InfinityStaker.rageQuit() uses the INFINITY_TREASURY variable to determine the recipient for the penalty tokens. As this variable is set to address(0) and ERC20.transfer reverts for a zero-address recipient, the function reverts and the user is not able to rage-quit.

Proof of Concept

InfinityStaker.updateInfinityTreasury

/// @dev Admin function to update Infinity treasury
  function updateInfinityTreasury(address _infinityTreasury) external onlyOwner {
    INFINITY_TREASURY = _infinityTreasury;
  }

InfinityStaker.rageQuit

function rageQuit() external override nonReentrant {
    (uint256 totalToUser, uint256 penalty) = getRageQuitAmounts(msg.sender);
    // update storage
    _clearUserStakedAmounts(msg.sender);
    // perform transfers
    IERC20(INFINITY_TOKEN).safeTransfer(msg.sender, totalToUser);
    IERC20(INFINITY_TOKEN).safeTransfer(INFINITY_TREASURY, penalty); // @audit-info reverts for zero-address recipient
    // emit event
    emit RageQuit(msg.sender, totalToUser, penalty);
}

Tools Used

Manual review

Recommended mitigation steps

Add a zero-address check for _infinityTreasury in InfinityStaker.updateInfinityTreasury.

nneverlander commented 2 years ago

Removed use of safeTransfer function

nneverlander commented 2 years ago

also added 0 check in the update function

HardlyDifficult commented 2 years ago

Agree this is best prevented. But it assumes a malicious owner and if they were to abuse this, it would devalue their own token. If unintentional, it could be promptly corrected. Lowering risk and merging with the warden's QA report #345

code-423n4 / 2022-06-infinity-findings