M-07 Unmitigated

[c4-submissions]

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.

[toshiSat]

We will be blocking zero address from manager once set to resolve this issue

[elmutt]

Votium strategy isnt intended to be used in a standalone fashion so it should be fine. We arent going to make this change as it would break many of our legacy votium strategy tests that we feel still have a lot of value.

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as new finding

[c4-judge]

0xleastwood marked the issue as duplicate of #51