Swap functionality to sell rewards is too permissive and could cause accidental or intentional loss of value

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L272-L305

Vulnerability details

Summary

While the intention is to use the 0x protocol to sell rewards, the implementation doesn't provide any basic guarantee this will correctly happen and grants the rewarder arbitrary control over the tokens held by the strategy.

Impact

Rewards earned in the VotingStrategy contract are exchanged for ETH and deposited back into the protocol. As indicated by the documentation, the intention is to swap these rewards for ETH using the 0x protocol:

Votium rewards are claimed with claimRewards() using merkle proofs published by votium every 2 weeks. applyRewards() sells rewards on 0x and deposits them back into afEth (and ultimately back into the safEth & votium strategies), making the afEth price go up.

However, the implementation of applyRewards() shallowly executes a series of calls to arbitrary targets:

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L272-L305

272:     function applyRewards(SwapData[] calldata _swapsData) public onlyRewarder {
273:         uint256 ethBalanceBefore = address(this).balance;
274:         for (uint256 i = 0; i < _swapsData.length; i++) {
275:             // Some tokens do not allow approval if allowance already exists
276:             uint256 allowance = IERC20(_swapsData[i].sellToken).allowance(
277:                 address(this),
278:                 address(_swapsData[i].spender)
279:             );
280:             if (allowance != type(uint256).max) {
281:                 if (allowance > 0) {
282:                     IERC20(_swapsData[i].sellToken).approve(
283:                         address(_swapsData[i].spender),
284:                         0
285:                     );
286:                 }
287:                 IERC20(_swapsData[i].sellToken).approve(
288:                     address(_swapsData[i].spender),
289:                     type(uint256).max
290:                 );
291:             }
292:             (bool success, ) = _swapsData[i].swapTarget.call(
293:                 _swapsData[i].swapCallData
294:             );
295:             if (!success) {
296:                 emit FailedToSell(_swapsData[i].sellToken);
297:             }
298:         }
299:         uint256 ethBalanceAfter = address(this).balance;
300:         uint256 ethReceived = ethBalanceAfter - ethBalanceBefore;
301: 
302:         if (address(manager) != address(0))
303:             IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
304:         else depositRewards(ethReceived);
305:     }

This not only fails to provide any guarantee that 0x will be used (and that it will be used correctly), but grants a lot of power to the rewarder which can be used accidentally or purposely to negatively impact the protocol. The rewarded role can grant any token approval to any spender and execute arbitrary calls on behalf of the VotingStrategy.

Recommendation

Provide better guarantees in the implementation of applyRewards() that 0x will be used to swap rewards, to ensure a more transparent and less error prone solution.

• Instead of granting arbitrary allowance to any spender, set this to the 0x entrypoint.
• Change arbitrary calls to the 0x protocol entrypoint.
• Data sent to the 0x contract could also be validated, for example to ensure the output token is ETH.

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed