Platform: Code4rena
Start Date: 09/11/2021
Pot Size: $75,000 USDC
Total HM: 57
Participants: 27
Period: 7 days
Judge: alcueca
Total Solo HM: 49
Id: 52
League: ETH
Rank: 11/27
Findings: 4
Award: $2,491.12
🌟 Selected for report: 8
🚀 Solo Findings: 2
🌟 Selected for report: Reigada
1619.075 USDC - $1,619.07
Reigada
In the contract BasePool the mint function can be frontrun. This will assign the NFT to the attacker which later on he can burn it retrieving the corresponding _nativeAsset and _foreignAsset initially deposited by the frontrun victim. https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/BasePool.sol#L149-L194
User1 transfers 1000 _nativeAsset tokens and 1000 _foreignAsset tokens into the BasePool contract. User1 calls the BasePool.mint() function to retrieve his NFT. Attacker is constantly polling for an increase of the balance of _nativeAsset and _foreignAsset of the contract OR attacker is constantly scanning the mempool for mint() function calls. Attacker detects an increase of balance of _nativeAsset and _foreignAsset OR attacker detects a mint() function call in the mempool. Attacker frontruns the mint call and retrieves the NFT. Gets a NFT that is worth 1000 _nativeAssets and 1000 _foreignAssets. User1 gets a NFT that is worth 0 _nativeAssets and 0 _foreignAssets. Attacker burns the NFT retrieving the corresponding _nativeAsset and _foreignAsset initially deposited by the victim.
Manual testing
Include in the mint() function the transfer of _nativeAssets and _foreignAssets to the smart contract.
#0 - SamSteinGG
2021-11-25T11:58:14Z
The pool is meant to be utilized via the router or smart contracts and is not meant to be utilized directly. The exact same "flaw" exists in Uniswap V2 whereby if you transfer assets directly someone else can claim them on your behalf.
#1 - alcueca
2021-12-11T06:52:26Z
Ah, so this how you prevent direct access to the pools. The issue is valid due to lack of documentation on the usage of the router.
#2 - SamSteinGG
2021-12-16T12:06:45Z
Firstly, documentation related issues cannot constitute a high risk vulnerability. Secondly, this type of documentation does not exist in Uniswap V2 either. We advise this finding to be set to no risk.
🌟 Selected for report: Reigada
485.7225 USDC - $485.72
Reigada
In the contract StakingRewards, the stake function assume that the amount of stakingToken is transferred to the smart contract after calling the safeTransferFrom function (and thus it updates the _balances mapping). However, this may not be true if the stakingToken is a transfer-on-fee token or a deflationary/rebasing token, causing the received amount to be less than the accounted amount in the _balances mapping.
Same can be applied for the withdraw function.
Manual code review
Get the actual received amount by calculating the difference of token balance before and after the transfer. For example: uint256 balanceBefore = stakingToken.balanceOf(address(this)); stakingToken.safeTransferFrom(msg.sender, address(this), amount); uint256 receivedAmount = stakingToken.balanceOf(address(this)) - balanceBefore; _totalSupply = _totalSupply.add(receivedAmount); _balances[msg.sender] = _balances[msg.sender].add(receivedAmount);
#0 - 0xstormtrooper
2021-11-15T09:03:30Z
VADER / USDV fee on transfer will be removed
🌟 Selected for report: Reigada
161.9075 USDC - $161.91
Reigada
In the contract LinearVesting, the function claim() contains the following require statement: require(vester.start == 0, "LinearVesting::claim: Incorrect Vesting Type");
This require statement will only be met if msg.sender has not called vestFor(msg.sender, amount) previously. If that's the case vest[msg.sender].amount will equal 0, hence 0 will be written into the vestedAmount which will not pass the following require statement: require(vestedAmount != 0, "LinearVesting::claim: Nothing to claim");
The current smart contract code does not allow for those 2 conditions to be met which means that either this function should be removed or some logic is missing somewhere.
Manual code review
Check if the function LinearVesting.claim() or the smart contract is missing some logic otherwise, remove the claim() function.
#0 - SamSteinGG
2021-11-25T11:34:58Z
This is incorrect as a vesting initialized in the constructor will have a different state.
#1 - alcueca
2021-12-11T07:09:11Z
There is no documentation regarding the required initial state for vesters.
🌟 Selected for report: Reigada
Also found by: Meta0xNull, cmichel
43.715 USDC - $43.72
Reigada
Checking addresses against zero-address during initialization is a security best-practice. However, such checks are missing in multiple constructors. Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/BasePool.sol#L94-L104 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L40-L49
Manual testing
Add zero-address checks in all the constructors.
🌟 Selected for report: Reigada
68.651 USDC - $68.65
Reigada
The function XVader.leave(uint256 _shares) got the following code:
function leave(uint256 _shares) external { // Gets the amount of xVader in existence uint256 totalShares = totalSupply(); // Calculates the amount of vader the xVader is worth uint256 vaderAmount = ( _shares * vader.balanceOf(address(this)) ) / totalShares; _burn(msg.sender, _shares); vader.transfer(msg.sender, vaderAmount); }
The assigment to uint256 totalShares can be removed to save some gas.
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/x-vader/XVader.sol#L50-L60
Manual code review
Use the following code instead to save gas:
function leave(uint256 _shares) external { // Calculates the amount of vader the xVader is worth uint256 vaderAmount = ( _shares * vader.balanceOf(address(this)) ) / totalSupply(); _burn(msg.sender, _shares); vader.transfer(msg.sender, vaderAmount); }
30.893 USDC - $30.89
Reigada
The contract StakingRewards is using SafeMath with a pragma ^0.8.0. This makes no sense and just causes more gas to be used as versions ^0.8.0 of Solidity already check for overflows/underflows.
https://github.com/ethereum/solidity/releases/tag/v0.8.0
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L2 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L19
Manual testing
Remove SafeMath and modify the mathematical operations to use the standard math operators.
🌟 Selected for report: Reigada
68.651 USDC - $68.65
Reigada
In all the loops, the variable i is incremented using i++. It is known that using ++i costs less gas per iteration than i++.
tokens/vesting/LinearVesting.sol:72: for (uint256 i = 0; i < vesters.length; i++) { tokens/Vader.sol:334: // for (uint256 i = 0; i < eras; i++) governance/GovernorAlpha.sol:437: for (uint256 i = 0; i < length; i++) { governance/GovernorAlpha.sol:466: for (uint256 i = 0; i < length; i++) { governance/GovernorAlpha.sol:571: for (uint256 i = 0; i < _targets.length; i++) { governance/GovernorAlpha.sol:619: for (uint256 i = 0; i < length; i++) { external/libraries/UniswapV2Library.sol:121: for (uint256 i; i < path.length - 1; i++) { twap/TwapOracle.sol:120: for (uint256 i = 0; i < pairCount; i++) { twap/TwapOracle.sol:326: for (uint256 i = 0; i < pairCount; i++) {
Manual testing
Use ++i instead of i++ to increment the value of an uint variable.
🌟 Selected for report: Reigada
Also found by: Meta0xNull, pants, pauliax
12.5116 USDC - $12.51
Reigada
As i is an uint, it is already initialized to 0. uint i = 0 reassings the 0 to i which wastes gas.
tokens/vesting/LinearVesting.sol:72: for (uint256 i = 0; i < vesters.length; i++) { tokens/Vader.sol:334: // for (uint256 i = 0; i < eras; i++) governance/GovernorAlpha.sol:437: for (uint256 i = 0; i < length; i++) { governance/GovernorAlpha.sol:466: for (uint256 i = 0; i < length; i++) { governance/GovernorAlpha.sol:571: for (uint256 i = 0; i < _targets.length; i++) { governance/GovernorAlpha.sol:619: for (uint256 i = 0; i < length; i++) { twap/TwapOracle.sol:120: for (uint256 i = 0; i < pairCount; i++) { twap/TwapOracle.sol:326: for (uint256 i = 0; i < pairCount; i++) {
Manual testing
Do not initialize i variable to 0 -> for (uint256 i; i < pairCount; i++) {