Platform: Code4rena
Start Date: 04/11/2021
Pot Size: $50,000 USDC
Total HM: 20
Participants: 28
Period: 7 days
Judge: 0xean
Total Solo HM: 11
Id: 51
League: ETH
Rank: 7/28
Findings: 5
Award: $2,686.78
🌟 Selected for report: 7
🚀 Solo Findings: 0
1308.4274 USDC - $1,308.43
cmichel
The protocol uses two amplifier values A1 and A2 for the swap, depending on the target price, see SwapUtils.determineA.
The swap curve is therefore a join of two different curves at the target price.
When doing a trade that crosses the target price, it should first perform the trade partially with A1 up to the target price, and then the rest of the trade order with A2.
However, the SwapUtils.swap / _calculateSwap function does not do this, it only uses the "new A", see getYC step 5.
// 5. Check if we switched A's during the swap if (aNew == a){ // We have used the correct A return y; } else { // We have switched A's, do it again with the new A return getY(self, tokenIndexFrom, tokenIndexTo, x, xp, aNew, d); }
Trades that cross the target price and would lead to a new amplifier being used are not split up and use the new amplifier for the entire trade. This can lead to a worse (better) average execution price than manually splitting the trade into two transactions, first up to but below the target price, and a second one with the rest of the trader order size, using both A1 and A2 values.
In the worst case, it could even be possible to make the entire trade with one amplifier and then sell the swap result again using the other amplifier making a profit.
Trades that lead to a change in amplifier value need to be split up into two trades using both amplifiers to correctly calculate the swap result.
cmichel
The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter should checked for success.
Some functions perform ERC20 transfers without checking for the return value:
BasicSale._processWithdrawalAirdropDistribution.claimInvestorDistribution.dev_rugpullAs the trusted mainToken token is used which supposedly reverts on failed transfers, not checking the return value does not lead to any security issues.
We still recommend checking it to abide by the EIP20 standard.
Consider using require(mainToken.transfer(_member, v_value), "transfer failed") instead.
#0 - chickenpie347
2021-11-16T14:17:56Z
Addressed in #31 .
cmichel
The idea of revoking vesting supposedly exists for the admins to call Vesting.revoke and claim back a user's vesting.
However, if the user wants to protect their vesting from being revoked by the admin, they can create a new vest with _isRevocable = false (and an amount of 0 or 1 wei) and this revocation status applies to all vestings.
Anyone can change the revocability status of all vestings of a user.
A user can frontrun an admin's revoke and make it fail, protecting their unvested amount from being sent to the multiSig instead of them.
The revoke status is currently vesting-independent, it's global for a user as benRevocable[_beneficiary] is only indexed by the user.
It should be indexed by the user and a specific vesting.
#0 - chickenpie347
2021-11-16T13:49:01Z
We actually had the revocable status per vesting, but we removed it to save gas for all other users (revocable would be a small minority compared to actual vests). While the assumption is correct, that the attacker - in this case, which would be a team member - can try to stop us from revoking, we could simply reverse it with a new vest and claim back tokens.
User blocking revoke() would only give them some more time, while the tokens continue accruing on schedule, but revoke() once successfully called drains all future unvested tokens.
#1 - chickenpie347
2021-11-16T13:53:05Z
Addressed in #132.
🌟 Selected for report: cmichel
290.7617 USDC - $290.76
cmichel
Some parameters of functions are not checked for invalid values:
Swap.setAdminFee: The newAdminFee should be validated the same way as in the constructorSwap.setSwapFee: The newSwapFee should be validated the same way as in the constructorSwap.setDefaultWithdrawFee: The newWithdrawFee should be validated the same way as in the constructorWrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy or wasted gas.
Validate the parameters.
🌟 Selected for report: cmichel
290.7617 USDC - $290.76
cmichel
The BasicSale contract includes ERC20 code like _balances, _allowances storage variables and Transfer, Approval events.
This code is never used.
Unused code can hint at programming or architectural errors.
Use it or remove it.
🌟 Selected for report: cmichel
290.7617 USDC - $290.76
cmichel
The BasicSale contract uses a secondsPerDay value of 84200 but one day has 86400 seconds.
The secondsPerDay does not reflect seconds per day.
Change the value.
🌟 Selected for report: cmichel
290.7617 USDC - $290.76
cmichel
The BasicSale.withdrawShareForMember function allows withdrawing the shares of other users.
While the tokens are sent to the correct address, this can lead to issues with smart contracts that might rely on claiming the tokens themselves.
As an example, suppose the member address corresponds to a smart contract that has a function of the following form:
function claimAndDoSomething() { uint256 claimedAmount = mainToken.balanceOf(address(this)); contract.withdrawShare(); claimedAmount = mainToken.balanceOf(address(this)) - claimedAmount; mainToken.transfer(externalWallet, claimedAmount); }
If the contract has no other functions to transfer out funds, they may be locked forever in this contract.
Do not allow users to withdraw on behalf of other users.
#0 - chickenpie347
2022-01-04T01:07:44Z
This function is not intended to be used by smart contracts, and if indeed smart contracts are created to be used, I am sure they would follow the intricacies needed.
#1 - 0xean
2022-01-08T03:21:22Z
I am not clear on what the benefit of allowing someone to withdraw on behalf of someone else actually is here. Will leave the issue as low risk
🌟 Selected for report: Meta0xNull
12.1196 USDC - $12.12
cmichel
The AirdropDistribution.validate function iterates over all airdropArray.
The array size is determined by the number of airdrop participants.
The function is very inefficient as it needs to iterate over all elements of the array.
The transactions can also fail if the arrays get too big and the transaction would consume more gas than the block limit. This will then result in a denial of service for the desired functionality and break core functionality.
Don't add new airdrop receivers and have validate accept the correct index already (computed by the frontend) such that it does not need to find the index on-chain.
#0 - chickenpie347
2022-01-04T01:05:53Z
Duplicate of #195
🌟 Selected for report: cmichel
44.8876 USDC - $44.89
cmichel
The Swap.constructor checks if both arrays _pooledTokens and decimals are of length two, but then does another check if these arrays have the same length.
require( _pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch" );
This check will always be true as it has been checked that both arrays are of length two.
8.1808 USDC - $8.18
cmichel
The AirdropDistribution.claimExact and InvestorDistribution.claimExact functions check that msg.sender != address(0).
This is always true, nobody has the private key of the zero address and it cannot be spoofed. This check can be removed.
12.1196 USDC - $12.12
cmichel
The AirdropDistribution._updateEmission and InvestorDistribution._updateEmission functions only perform code if block.timestamp >= startEpochTime + RATE_TIME but this function is only called from updateEmission if this is true.
Therefore this if condition will always be true and is not necessary.
It should be removed to save gas (or be replaced with an assert if you want to ensure this is the case.)
#0 - 0xean
2022-01-08T22:45:49Z
duplicate of #34