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: 10/10
Findings: 6
Award: $1,759.15
🌟 Selected for report: 7
🚀 Solo Findings: 0
2.6398 VETH - $137.27
0.0634 ETH - $158.39
gpersoon
The function deploySynth of Pools.sol has the following require: require(token != VADER || token != USDV); This require never stops execution because the following statement is always true: (token != VADER || token != USDV)
Pools.sol: function deploySynth(address token) external { require(token != VADER || token != USDV); iFACTORY(FACTORY).deploySynth(token); }
Editor
The statement should be: require(token != VADER && token != USDV);
#1 - dmvt
2021-05-26T22:49:04Z
duplicate of #124
1.7819 VETH - $92.66
0.0428 ETH - $106.91
gpersoon
Most of the solidity contracts have an init function that everyone can call. This could lead to a race condition when the contract is deployed. At that moment a hacker could call the init function and make the deployed contracts useless. Then it would have to be redeployed, costing a lot of gas.
DAO.sol: function init(address _vader, address _usdv, address _vault) public { Factory.sol: function init(address _pool) public { Pools.sol: function init(address _vader, address _usdv, address _router, address _factory) public { Router.sol: function init(address _vader, address _usdv, address _pool) public { USDV.sol: function init(address _vader, address _vault, address _router) external { Utils.sol: function init(address _vader, address _usdv, address _router, address _pools, address _factory) public { Vader.sol: function init(address _vether, address _USDV, address _utils) external { Vault.sol: function init(address _vader, address _usdv, address _router, address _factory, address _pool) public {
Editor
Add a check to the init function, for example that only the deployer can call the function.
#0 - strictly-scarce
2021-04-26T15:05:59Z
Yes, but only once. Could add a deployer check tho
#1 - dmvt
2021-05-25T10:42:47Z
After considerable evaluation and seeing the wide range of threat factors that were put forward by wardens related to this issue, I've decided that the potential threat here does extend beyond gas.
A worst case scenario could cause significant damage.
It is extremely unlikely that an attacker could successfully time this type of attack.
An attacker would have to successfully intercept more than one init due to the highly coupled nature of the contract. If they did so incorrectly, the entire system would not function. Presuming they succeeded, the team would also have to overlook the failure of or forget to make multiple critical transaction calls in their deployment scripts. To realize significant financial gains, the attacker would have to leave their exploit code in place for an extended period of time.
The likelihood is extremely low, but the impact would be critical. For this reason, I'm normalizing all of these reports to a Medium Risk.
🌟 Selected for report: gpersoon
3.259 VETH - $169.47
0.0782 ETH - $195.54
gpersoon
Vether.sol (https://etherscan.io/address/0x4Ba6dDd7b89ed838FEd25d208D4f644106E34279#code) is the 4th contract version. It takes the totalBurnt value of the 2nd version of the contract an continues on that. It seem more logical to use the totalBurnt value of the 3rd version of the contract an continues on that. This way the value of totalBurnt is probably not the real value.
vether.sol:
contract Vether4 is ERC20 {
constructor() public {
...
vether2 = 0x01217729940055011F17BeFE6270e6E59B7d0337; // Second Vether
vether3 = 0x75572098dc462F976127f59F8c97dFa291f81d8b;
...
totalBurnt = VETH(vether2).totalBurnt(); totalFees = 0;
Editor
Check if indeed vether3 should be used and update the code to use vether3
#0 - strictly-scarce
2021-04-26T13:41:58Z
Vether is deployed code and can't be changed.
#1 - Mervyn853
2021-05-01T08:47:04Z
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: 0
0 VETH - $0.00
0.0116 ETH - $29.09
gpersoon
Several variables are initialized with a constant value are never changed. Several other variables (especially address of contracts) are initialized a value are never changed. These could very well be chanced to constant or immutable. This reduced the risk on accidental changes of the values and also saves a bit of gas.
Router.sol function init(address _vader, address _usdv, address _pool) public { require(inited == false, "inited"); inited = true; VADER = _vader; USDV = _usdv; POOLS = _pool;
vader.sol constructor() { name = 'VADER PROTOCOL TOKEN'; symbol = 'VADER'; decimals = 18; _1m = 10**6 * 10 ** decimals; //1m baseline = _1m; totalSupply = 0; maxSupply = 2 * _1m; currentEra = 1; secondsPerEra = 1; //86400; nextEraTime = block.timestamp + secondsPerEra; emissionCurve = 900; DAO = msg.sender; burnAddress = 0x0111011001100001011011000111010101100101; }
Editor
Change variables to constant or immutable where possible.
#0 - 0xBrian
2021-05-11T05:23:45Z
#1 - dmvt
2021-05-26T21:10:28Z
duplicate of #286
🌟 Selected for report: gpersoon
0 VETH - $0.00
0.0638 ETH - $159.60
gpersoon
There is some unused / redundant code present.
Router.sol defines repayDelay but it is never used Vault.sol initializes POOLS twice, with the same value.
Router.sol: uint public repayDelay = 3600;
Vault.sol:
function init(address _vader, address _usdv, address _router, ...
..
POOLS = _pool;
..
POOLS = _pool;
Editor
Remove redundant code
#0 - 0xBrian
2021-05-11T04:21:52Z
🌟 Selected for report: gpersoon
0 VETH - $0.00
0.0638 ETH - $159.60
gpersoon
The function sortArray is only called by getAnchorPrice() in Router.sol Then it is only used to get the second element of the sorted array. With this knowledge it is possible to write a function that is more efficient. Sort functions are relative expensive functions.
Router.sol:
function getAnchorPrice() public view returns (uint anchorPrice) {
....
uint[] memory _sortedAnchorFeed = iUTILS(UTILS()).sortArray(arrayPrices);
anchorPrice = _sortedAnchorFeed[2];
Make a partial sort function that only returns the required value
#0 - strictly-scarce
2021-04-26T15:05:21Z
Do you have an example of how to make it better? It's already pretty good for what it does
🌟 Selected for report: gpersoon
0 VETH - $0.00
0.0638 ETH - $159.60
gpersoon
The function transferOut of Pools.sol contains a iERC20(_token).transfer where the result of the function isn't checked. This could result in transfers that don't succeed are undetected.
Pools.sol: function transferOut(address _token, uint _amount, address _recipient) internal { if(_token == VADER){ pooledVADER = pooledVADER - _amount; // Accounting } else if(_token == USDV) { pooledUSDV = pooledUSDV - _amount; // Accounting } if(_recipient != address(this)){ iERC20(_token).transfer(_recipient, _amount); } }
Editor
Add a require statement to check the result: require(...transfer(...) )
#0 - 0xBrian
2021-05-11T06:08:10Z
🌟 Selected for report: gpersoon
0 VETH - $0.00
0.0638 ETH - $159.60
gpersoon
The function _transfer of Vether.sol contains are relative complicated statement to determine if an emit should be done for the _fee. This could be simplified, which saves a bit of gas and is also easier to read
function _transfer(address _from, address _to, uint _value) private { require(_balances[_from] >= _value, 'Must not send more than balance'); require(_balances[_to] + _value >= _balances[_to], 'Balance overflow'); _balances[_from] =_balances[_from].sub(_value); uint _fee = _getFee(_from, _to, _value); // Get fee amount _balances[_to] += (_value.sub(_fee)); // Add to receiver _balances[address(this)] += _fee; // Add fee to self totalFees += _fee; // Track fees collected emit Transfer(_from, _to, (_value.sub(_fee))); // Transfer event if (!mapAddress_Excluded[_from] && !mapAddress_Excluded[_to]) { emit Transfer(_from, address(this), _fee); // Fee Transfer event } }
Editor
if (!mapAddress_Excluded[_from] && !mapAddress_Excluded[_to]) { can be replaced with: if (_fee > 0) {
#0 - strictly-scarce
2021-04-26T14:00:11Z
Vether is deployed code and can't be changed
🌟 Selected for report: gpersoon
0 VETH - $0.00
0.0638 ETH - $159.60
gpersoon
In the function addExcluded of Vether.sol (https://etherscan.io/address/0x4Ba6dDd7b89ed838FEd25d208D4f644106E34279#code) a call is made to _transfer to pay a fee of 128 Vether, however to do that _transfer you have to pay an additional fee of 0.128 Vether. Right after this call mapAddress_Excluded is set, which prevents paying fees. It seems logical to just pay fees once.
function addExcluded(address excluded) external { if(!mapAddress_Excluded[excluded]){ _transfer(msg.sender, address(this), mapEra_Emission[1]/16); // Pay fee of 128 Vether mapAddress_Excluded[excluded] = true; // Add desired address excludedArray.push(excluded); excludedCount +=1; // Record details totalFees += mapEra_Emission[1]/16; // Record fees } }
Editor
Check if the conclusions are sound. Then do the mapAddress_Excluded[excluded] = true; ... before the _transfer
#0 - strictly-scarce
2021-04-26T13:57:39Z
Vether is deployed code and can't be changed