Platform: Code4rena
Start Date: 22/04/2021
Pot Size: $120,000 USDC
Total HM: 41
Participants: 10
Period: 7 days
Judge: LSDan
Total Solo HM: 28
Id: 5
League: ETH
Rank: 3/10
Findings: 14
Award: $7,190.26
🌟 Selected for report: 14
🚀 Solo Findings: 0
5.9395 VETH - $308.86
0.1425 ETH - $356.37
paulius.eth
Anyone can call functions lockUnits and unlockUnits (not only router) as it does not have any authorization checks. Thus it is possible to set any values for an account and thus make functions that rely on these values misbehave or fail.
Add authorization so only the intended entities would be able to lock and unlock units.
#0 - strictly-scarce
2021-05-01T07:35:03Z
#1 - 0xBrian
2021-05-11T08:13:39Z
#2 - dmvt
2021-05-26T22:23:19Z
duplicate of #208
14.6655 VETH - $762.61
0.352 ETH - $879.93
paulius.eth
function _convert only performs the conversion when minting is turned on: if(minting()){ However, the funds are collected before and it does not reimburse the sender: function convertForMember(address member, uint amount) public returns(uint) { getFunds(VADER, amount); return _convert(member, amount); } Same situation with function redeemForMember. I see no reason why the user should send and lose his tokens when the minting is turned off.
Probably it would be better to replace "if" with "require" so that users won't be tricked into such an accident.
#0 - strictly-scarce
2021-05-01T13:31:28Z
#1 - dmvt
2021-05-26T22:09:47Z
duplicate of #238
2.6398 VETH - $137.27
0.0634 ETH - $158.39
paulius.eth
function completeProposal which is the last step sets mapPID_finalised to true and resets mapPID_finalising to false. Function voteProposal only checks that mapPID_finalising is false, it does not check that the mapPID_finalised, thus the same proposal can be voted again, then finalized and executed.
voteProposal should require that mapPID_finalised is false.
#0 - strictly-scarce
2021-05-01T07:32:25Z
Valid attack path, although questionable if high-risk since funds-not-at-risk
#1 - Mervyn853
2021-05-01T08:17:35Z
Our decision matrix for severity:
0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.
Recommended: 2
#2 - 0xBrian
2021-05-11T04:27:19Z
Unused mapPID_finalised addressed https://github.com/vetherasset/vaderprotocol-contracts/commit/6f961e6cd159e905ef53a5a067f956d21f8a46ee
#3 - 0xBrian
2021-05-11T08:36:10Z
Well, the unused mapPID_finalised was addressed, but this issue probably still remains.
#4 - dmvt
2021-05-26T22:46:32Z
duplicate of #229
2.6398 VETH - $137.27
0.0634 ETH - $158.39
paulius.eth
function deploySynth checks that token is not VADER or not USDV. The condition should be && (not ||) as OR condition always holds when VADER != USDV: function deploySynth(address token) external { require(token != VADER || token != USDV); iFACTORY(FACTORY).deploySynth(token); }
The condition should be &&, not ||.
#0 - strictly-scarce
2021-05-01T13:19:13Z
#2 - dmvt
2021-05-26T22:49:08Z
duplicate of #124
4.3997 VETH - $228.78
0.1056 ETH - $263.98
paulius.eth
identical if-else branches (redundant code):
if(iPOOLS(POOLS).isAsset(iSYNTH(synth).TOKEN())){
uint _adjustedReserve = iROUTER(ROUTER).getUSDVAmount(reserveVADER()) + reserveUSDV(); // Aggregrate reserves
return iUTILS(UTILS()).calcShare(_weight, totalWeight, _adjustedReserve / erasToEarn); // Get member's share of that
} else{
uint _adjustedReserve = iROUTER(ROUTER).getUSDVAmount(reserveVADER()) + reserveUSDV();
return iUTILS(UTILS()).calcShare(_weight, totalWeight, _adjustedReserve / erasToEarn);
}
#0 - dmvt
2021-05-25T14:41:35Z
duplicate of #51
🌟 Selected for report: pauliax
3.259 VETH - $169.47
0.0782 ETH - $195.54
paulius.eth
contract Vether4 function _recordBurn does not check that _eth > 0, thus it is possible to pass this check multiple times: if (mapEraDay_MemberUnits[_era][_day][_member] == 0) If the user hasn't contributed to this day yet, it updates mapMemberEra_Days, mapEraDay_MemberCount, and mapEraDay_Members. However, when msg.value is 0 it is possible to trigger this condition again and again as mapEraDay_MemberUnits still remains 0.
Either do not allow burns of 0 _eth or add an extra check in the if statement.
0.594 VETH - $30.89
0.0143 ETH - $35.64
paulius.eth
contract Router. anchorPrice = _sortedAnchorFeed[2]; // Return the middle Here it hardcodes the middle index, so it assumes that it never changes. However, anchorLimit can be changed by function setAnchorParams. Also, I think it doesn't make sense to allow setting anchorLimit to a lower value than it was before because there is no possibility to remove the anchor and other functions will still iterate over arrayAnchors.length.
Better use arrayAnchors.length / 2 as the middle price.
#0 - dmvt
2021-05-26T22:40:29Z
duplicate of #213
🌟 Selected for report: pauliax
3.259 VETH - $169.47
0.0782 ETH - $195.54
paulius.eth
function curatePool emits Curated event every time. It should emit this event only when the conditions are fulfilled.
Put this event inside the most inner if block.
🌟 Selected for report: pauliax
3.259 VETH - $169.47
0.0782 ETH - $195.54
paulius.eth
function listAnchor sets _isCurated to true but does not update the curatedPoolCount and does not emit the Curated event. I don't see this curatedPoolCount variable used anywhere so probably it's just needed for the frontend consumption.
I think the best solution would be to replace _isCurated[token] = true; with a call to a function curatePool. It also skips if the same anchor is listed twice.
1.4666 VETH - $76.26
0.0352 ETH - $87.99
paulius.eth
In contract USDV blockDelay is not initialized and needs to be explicitly set by calling function setParams. Otherwise, it gets a default value of 0 so flashProof is not effective unless the value is set.
It depends on the intentions, you can initialize it in the constructor (or init function) or maybe this precaution is intended to be turned on later.
0 VETH - $0.00
0.0116 ETH - $29.09
paulius.eth
Both structs CollateralDetails and DebtDetails have unused ID field which is never set nor queried: uint ID;
#0 - 0xBrian
2021-05-11T05:30:20Z
🌟 Selected for report: pauliax
3.259 VETH - $169.47
0.0782 ETH - $195.54
paulius.eth
As contract Vether4 is using pragma solidity 0.6.4; SafeMath is not enabled by default, thus making this check inside function distribute avoidable (overflow): upgradedAmount += ownership[i]; require(upgradedAmount <= maxEmissions, "Must not send more than possible"); Of course, this function can only be called by the deployer (who is later expected to call purgeDeployer) so the issue is only theoretical.
Use SafeMath here or just be informed about this theoretical issue.
0 VETH - $0.00
0.0116 ETH - $29.09
paulius.eth
There are variables that are only assigned once (e.g. in a constructor). You should mark such variables with the keyword "immutable", this greatly reduces the gas costs. A concrete example of such a variable is "VADER" which is only initialized once and cannot be changed later: VADER = _vader; There are plenty of such variables across the contracts.
#0 - 0xBrian
2021-05-11T05:21:14Z
0 VETH - $0.00
0.0084 ETH - $20.94
paulius.eth
A variable in function newGrantProposal could be extracted as a constant as it never changes: string memory typeStr = "GRANT";
#0 - dmvt
2021-05-20T20:21:44Z
duplicate of #129
0 VETH - $0.00
0.0116 ETH - $29.09
paulius.eth
function finaliseProposal contains a line that can never be triggered: if(!hasQuorum(proposalID)){ _finalise(proposalID); } This is because it has a check above which makes sure that the proposal is already in the finalizing state: require(mapPID_finalising[proposalID] == true, "Must be finalising"); The only place where mapPID_finalising is set to true is function _finalise. What is even more strange is that the comment above the function says: "Proposal with quorum can finalise after cool off period", however, the actual check is !hasQuorum.
Remove this unreachable branch or update guard conditions.
#0 - 0xBrian
2021-05-11T04:24:46Z
#1 - dmvt
2021-05-26T21:57:13Z
duplicate of #186
0 VETH - $0.00
0.0172 ETH - $43.09
paulius.eth
mapping(address => bool) _isMember; is never set nor updated so function isMember always returns false.
Either remove this unused mapping or make use of it depending on the intentions.
#0 - 0xBrian
2021-05-11T04:30:00Z
#1 - dmvt
2021-05-20T21:00:43Z
duplicate of #118
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
Here are some useless calculations: if(_token == VADER && _pool != VADER){ // Want to know added VADER addedAmount = _balance - pooledVADER; pooledVADER = pooledVADER + addedAmount; } else if(_token == USDV) { // Want to know added USDV addedAmount = _balance - pooledUSDV; pooledUSDV = pooledUSDV + addedAmount; if you do the simple maths, it is always in the first case, pooledVADER = _balance, in the second case pooledUSDV = _balance.
#0 - 0xBrian
2021-05-11T06:11:22Z
0 VETH - $0.00
0.0172 ETH - $43.09
paulius.eth
flip functions (flipEmissions and flipMinting) can be refactored to one liner to save some gas and improve readability, for example: function flipEmissions() external onlyDAO { emitting = !emitting; }
#1 - dmvt
2021-05-26T22:06:20Z
duplicate of #197
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
Mappings are expensive. All PID related variables could be extracted to a struct: mapping(uint => GrantDetails) public mapPID_grant; mapping(uint => address) public mapPID_address; mapping(uint => string) public mapPID_type; mapping(uint => uint) public mapPID_votes; mapping(uint => uint) public mapPID_timeStart; mapping(uint => bool) public mapPID_finalising; mapping(uint => bool) public mapPID_finalised; mapping(uint => mapping(address => uint)) public mapPIDMember_votes; By the way, mapPID_finalised is set but not used anywhere.
#0 - 0xBrian
2021-05-11T04:27:02Z
Unused mapPID_finalised addressed https://github.com/vetherasset/vaderprotocol-contracts/commit/6f961e6cd159e905ef53a5a067f956d21f8a46ee
#1 - dmvt
2021-05-20T19:43:31Z
@0xBrian note that this was not addressed by the link provided
#2 - 0xBrian
2021-05-21T04:19:01Z
@dmvt Sorry, I only meant to note that the mapPID_finalised issue was addressed. The other issue about saving gas by extracting to a struct, whatever that means, still remains to be considered.
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
a bit cheapier when you replace: require(inited == false); with: require(!inited); same with variable == true.
#0 - 0xBrian
2021-05-11T05:55:19Z
At some point we got rid of all comparisons with boolean literals.
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
There are several duplicate calls or storage access that can be cached. For example, here, iSYNTH(_synth).TOKEN() is called twice: uint _weight = iUTILS(UTILS()).calcValueInBase(iSYNTH(_synth).TOKEN(), _amount); if(iPOOLS(POOLS).isAnchor(iSYNTH(_synth).TOKEN()) or here it calculates _value.sub(_fee) twice: // Get fee amount _balances[_to] += (_value.sub(_fee)); ... emit Transfer(_from, _to, (_value.sub(_fee))); or: uint _synthUnits = iUTILS(UTILS()).calcSynthUnits(_actualInputBase, mapToken_baseAmount[token], mapToken_Units[token]); // Get Units outputAmount = iUTILS(UTILS()).calcSwapOutput(_actualInputBase, mapToken_baseAmount[token], mapToken_tokenAmount[token]); // Get output
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
Could save some gas on every transfer if storage access of mapAddress_Excluded is replaced here: if (!mapAddress_Excluded[_from] && !mapAddress_Excluded[_to]) with: if (_fee > 0) then inside this if block you can also include this sentence: totalFees += _fee; also the initialization of totalFees is useless as it automatically gets assigned a default value of 0: totalFees = 0;
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
if you calculate the exclusion fee only once and store it in a variable, gas costs will be reduced: mapEra_Emission[1] / 16 event better, as mapEra_Emission[1] is set initially in the constructor, this fee amount could also be calculated in the constructor and stored in a constant variable as it never changes. even comment says "Pay fee of 128 Vether". Same here: uint maxEmissions = (upgradeHeight - 1) * mapEra_Emission[1];
0 VETH - $0.00
0.0287 ETH - $71.82
paulius.eth
Useless checking >= 0 as uint cannot go negative: require((bp <= 10000) && (bp >= 0), "Must be correct BP"); Same here:
if(_fee >= 0 && _fee <= amount){
#0 - 0xBrian
2021-05-11T06:12:29Z
At some point we got rid of the tautological uint >= 0 checks.
#1 - dmvt
2021-05-26T22:07:51Z
duplicate of #256
🌟 Selected for report: pauliax
0 VETH - $0.00
0.0638 ETH - $159.60
paulius.eth
In function updateAnchorPrice here 'arrayAnchors[i]' can be replaced with 'token' to eliminate one expensive storage access: arrayPrices[i] = iUTILS(UTILS()).calcValueInBase(arrayAnchors[i], one);
#0 - 0xBrian
2021-05-11T06:15:04Z