code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Heart.sol#L150 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Heart.sol#L106

Vulnerability details

Impact

Heart.beat() is designed to pay a reward when users call the function.

If the admin withdraws the all amount of rewardToken by fault, beat() function will revert because of this one.

Proof of Concept

withdrawUnspentRewards() is used to withdraw unspent tokens and we should prevent to withdraw current rewardToken when the contract is active.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend checking token_ != rewardToken or 'active == false' like below.

function withdrawUnspentRewards(ERC20 token_) external onlyRole("heart_admin") { 
    require(token_ != rewardToken || !active, "Can't withdraw rewardToken"); //++++++++++++++++

    token_.safeTransfer(msg.sender, token_.balanceOf(address(this)));
}

Oighty commented 2 years ago

79 will be remediated, as such, there isn't really a risk here. See #313 for a related mitigation.

0xean commented 2 years ago

On the fence with this one, it's a different issue than #79 and ultimately comes down to the administrator of these contracts misconfiguring the rewards by removing them. It's not different than them setting the rewards to 0 or the contract address of the ERC20 used for rewards to an incorrect address.

That being said, I think QA is more appropriate for this. Yes, it could cause the protocol to stop functioning as intended, but ultimately it's not really a vulnerability any more than any sort of misconfiguration by the admins would be.