Open c4-submissions opened 1 year ago
@toshiSat I believe best solution is to get rid of _amount and only use msg.value here
0xleastwood marked the issue as primary issue
0xleastwood marked the issue as duplicate of #33
0xleastwood marked the issue as satisfactory
0xleastwood changed the severity to 2 (Med Risk)
0xleastwood changed the severity to QA (Quality Assurance)
@0xleastwood Liam, sorry, may I ask why this issue was downgraded? It seems it was linked somehow to #33 which got downgraded recently. The exposed functions here are part of both core contracts.
@0xleastwood Liam, sorry, may I ask why this issue was downgraded? It seems it was linked somehow to #33 which got downgraded recently. The exposed functions here are part of both core contracts.
Oops, I have no idea what happened here. This was not meant to be downgraded. Apologies.
This previously downgraded issue has been upgraded by 0xleastwood
Oh wait this was a dupe of an issue I downgraded. Let me confirm.
This is valid and I think should have been a standalone issue, it seems like anyone can arbitrarily spend native ether in exchange for locked cvx tokens. It is unclear how this might be abused because applyRewards()
will only transfer out new native ether which was received from its swaps.
0xleastwood marked the issue as unsatisfactory: Invalid
0xleastwood changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by 0xleastwood
0xleastwood changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by 0xleastwood
0xleastwood marked the issue as not a duplicate
0xleastwood removed the grade
0xleastwood marked the issue as selected for report
0xleastwood marked the issue as satisfactory
0xleastwood removed the grade
0xleastwood marked the issue as primary issue
@0xleastwood I mentioned this issue in L-08.
However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.
@0xleastwood I mentioned this issue in L-08.
However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.
These are not equivalent, you talk about the fact that _amount == msg.value
should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, the depositRewards()
function is missing access control.
@0xleastwood I mentioned this issue in L-08. However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.
These are not equivalent, you talk about the fact that
_amount == msg.value
should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, thedepositRewards()
function is missing access control.
And what is the impact according to #38?
The impact of not checking that _amount == msg.value
is that "any discrepancy is lost".
Otherwise, there is no ether in the contracts, so calling them does nothing. But if someone for some reason sends ether to the contract, then it is beneficial to the protocol if these functions are called so that the ether is deposited as rewards. Why should the public not be able to do this, which only benefits the protocol, on behalf of the owner? So what is the impact of the missing access control?
@0xleastwood I mentioned this issue in L-08. However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.
These are not equivalent, you talk about the fact that
_amount == msg.value
should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, thedepositRewards()
function is missing access control.And what is the impact according to #38? The impact of not checking that
_amount == msg.value
is that "any discrepancy is lost". Otherwise, there is no ether in the contracts, so calling them does nothing. But if someone for some reason sends ether to the contract, then it is beneficial to the protocol if these functions are called so that the ether is deposited as rewards. Why should the public not be able to do this, which only benefits the protocol, on behalf of the owner? So what is the impact of the missing access control?
I'm not stating any direct theft of funds (in fact it is mentioned in the report), hence the med severity. It's the combination of both issues that conflicts with the protocol specs:
depositRewards() is called by the votium strategy upon claiming rewards to make the afEth price go up by distributing funds into both strategies according to ratio.
Rewarder - Address of wallet that will handle calling the reward functions and swapping the tokens out
It's the combination of both issues that conflicts with the protocol specs:
Conflicting with protocol specs is QA.
It's the combination of both issues that conflicts with the protocol specs:
Conflicting with protocol specs is QA.
No, it's medium
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
Agree, it is medium.
@romeroadrian, "the function of the protocol or its availability could be impacted" is not the same a conflicting with specs; function and availability is not specs. In what way do you mean that the protocol stops working?
I believe this solves it based on original authors recommendation:
https://github.com/asymmetryfinance/afeth/pull/169
also
elmutt (sponsor) confirmed
Lines of code
https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L203 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L272
Vulnerability details
Summary
Some functions that are part of the Votium reward flow are left unprotected and can be accessed by anyone to spend resources held by the contract.
Impact
Rewards coming from the Votium protocol are claimed and compounded back in AfEth. This flow consists of two parts, both controlled and initiated by the rewarder role: first, rewards are claimed in Votium and Convex using
claimRewards()
, second, those rewards are swapped to ETH and deposited back in the protocol usingapplyRewards()
.After rewards are swapped, the VotiumStrategy will call AfEth to manage the deposited rewards, which eventually calls back the VotiumStrategy. These interactions are represented in the previous diagram as steps 5 and 6.
However, both of the functions that implement these steps are publicly accessible and don't have any validation over the amount of ETH sent. Let's first see the case of
AfEth::depositRewards()
:As we can see in the previous snippet of code, the function doesn't have any access control and doesn't check if the
_amount
parameter matches the amount of ETH being sent (msg.value
). Anyone can call this function with any amount value without actually sending any ETH value.The implementation of
depositRewards()
inVotiumStrategyCore
has the same issue:Any ETH held in these two contracts can be arbitrarily spent by any unauthorized account. The caller cannot remove value from here, unless sandwiching the trade or benefitting via a third-party call, but can use these functions to grief and unauthorizedly spend any ETH present in these contracts.
Recommendation
If these functions are indeed intended to be publicly accessible, then add a validation to ensure that the amount argument matches the callvalue sent, i.e.
require(_amount == msg.value)
.On the other hand, if these should only be part of the reward flow initiated by the rewarder role, then validate that
AfEth::depositRewards()
is called from the Votium Strategy (vEthAddress
), and validate thatVotiumStrategy::depositRewards()
is called either from AfEth (manager
) or internally throughapplyRewards()
.Assessed type
Access Control