Lines of code

Vulnerability details

Mitigation of M-07: Issue NOT fully mitigated with ERROR

Mitigated issue

M-07: Lack of access control and value validation in the reward flow exposes functions to public access

The issue was that anyone can deposit rewards to AfEth, and that if AfEth or VotiumStrategy hold ether anyone can deposit this as rewards.

Mitigation review

AfEth.depositRewards() now uses msg.value directly instead of a _amount parameter, so this function cannot be used to spend ether held by AfEth. It has also been access restricted so that only VotiumStrategy or the rewarder may deposit rewards to AfEth.
VotiumStrategyCore.depositRewards() retains its _amount parameter, because it needs to be able to spend the ether obtained in VotiumStrategyCore.applyRewards(), but has been access restricted by onlyManager,

modifier onlyManager() {
    if (address(manager) != address(0) && msg.sender != manager)
        revert NotManager();
    _;
}

We see that onlyManager is ineffective when manager == address(0) so that in this case VotiumStrategyCore.depositRewards() is publicly accessible and the issue remains.

Recommended correction

Change onlyManager into a onlyVManagerOrRewarder, like the onlyVotiumOrRewarder used for AfEth.depositRewards(). This means that the rewarder can directly call VotiumStrategyCore.depositRewards(). But since he is free to call AfEth.depositRewards() he might as well be allowed to call both, it seems. This way the rewarder has complete and sole external access to all rewards functionality.

Mitigation error - the ether is now stuck instead

The exploitable ether held by AfEth or VotiumStrategy could previously only be reached by applying this amount as rewards. Since this functionality has now been blocked, there is no way to access this ether and it is permanently stuck in the contract.

The recommended correction above allows the rewarder to make use of ether stuck in VotiumStrategy. But ether stuck in AfEth is rendered inaccessible by the new use of msg.value.

Best is probably to simply add a function to sweep stuck ether, or to amend withdrawStuckTokens() with such functionality.

---

The text was updated successfully, but these errors were encountered:

All reactions