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: 15/28
Findings: 3
Award: $698.95
🌟 Selected for report: 7
🚀 Solo Findings: 0
hyh
Griefing attack is possible for revoke mechanics by calling vest() with a tiny amount and zero _isRevocable. This will switch revocable off for the whole vesting amount (i.e. the whole set of timelocks flag is being set via last vest call).
And vice versa, whenever the vesting isn't revocable, a beneficiary can send a tiny amount to vest() for himself (i.e. do vest(me, 1 wei, 1)), switching the flag on.
The severity is medium as the attack is reversable this way and revoke() is owner executable only, but there is also a case of malicious owner, who can set _isRevocable to 1 via this vulnerability and surpass vesting via calling revoke() for the addresses associated with him. Both AirdropDistribution and InvestorDistribution call vest with 0 isRevocable, i.e. revoke mechanics isn't a part of standard process now.
benRevocable flag works for the whole vestings for an address. It can be switched on and off by anyone sending tiny _amount to vest() function and setting desired state of the flag. The flag is basically meaningless this way as anyone, including beneficiary, can switch it on and off. https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L83
As vest() function to be accessed by Distribution contracts only it is recommended to add an expandable list of allowed Distribution contracts and add an access modifier to vest() https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L73
Now: function vest(address _beneficiary, uint256 _amount, uint256 _isRevocable) external payable whenNotPaused {
To be: function vest(address _beneficiary, uint256 _amount, uint256 _isRevocable) external payable onlyDistribution whenNotPaused {
If manual vest/revoke by an Owner is planned, the Owner to be added to the Distribution list, but also benRevocable should be added to timelocks array and become per timelock based.
Now: struct Timelock { uint256 amount; uint256 releaseTimestamp; } mapping(address => Timelock[]) public timelocks; mapping(address => bool[2]) public benRevocable;
To be: struct Timelock { uint256 amount; uint256 releaseTimestamp; bool[2] benRevocable; } mapping(address => Timelock[]) public timelocks;
revoke() logic should be modified accordingly to affect allowed timelocks only.
#0 - chickenpie347
2021-11-16T13:50:52Z
This has been addressed in #220 and #213 and #132.
🌟 Selected for report: hyh
290.7617 USDC - $290.76
hyh
The only usage is in _claimableAmount function and can be rewritten with one uint256 storage variable. https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L183 benVested cannot be used to get current state as it is updated only during claim() and revoke() calls and calcClaimableAmount() to be used instead.
The timelocks totals and benTotal cannot differ as timelocks are updated and deleted in vest() and revoke() functions only correspondingly, while there benTotal is updated with very same amount without any additional conditions. https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L91 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L128
This way the 's <= benTotal[_addr]' check is redundant and to be removed.
's <= benTotal[_addr]' check can be dangerous: the totals and benTotal cannot differ, while if there would be such a possibility, various attacks might be possible, for example a griefing one, when claim() always fails because of this check, and so on.
I.e. now benVested can be simplified and check is not needed, while if there would be such a situation when it is needed, simple check as it is cannot be sufficient, and some code redesign should be done instead.
Code update: Now: mapping(address => uint256[2]) public benVested; ... uint256 completely_vested = 0; uint256 partial_sum = 0; ... completely_vested = completely_vested.add(timelocks[_addr][i].amount); ... partial_sum = partial_sum.add(claimable); ... benVested[_addr][0] = benVested[_addr][0].add(completely_vested); benVested[_addr][1] = partial_sum; uint256 s = benVested[_addr][0].add(partial_sum); assert(s <= benTotal[_addr]); return s;
To be: mapping(address => uint256) public benVested; ... uint256 currently_vested = 0; ... currently_vested = currently_vested.add(timelocks[_addr][i].amount); ... currently_vested = currently_vested.add(claimable); ... uint256 s = benVested[_addr].add(currently_vested); benVested[_addr] = s; return s;
Also, cleaning in revoke() simplifies to benVested[_addr] = 0; https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L127
20.1994 USDC - $20.20
hyh
Gas overspending due to excessive storage reads
SwapUtils's getD function reads xp[] from storage twice and can be simplified https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L620
Now: uint256 a = determineA(self, _xp(self)); // determine the correct A return getD(_xp(self), a); To be: uint256[] memory xP = _xp(self.balances, self.tokenPrecisionMultipliers); return getD(xP, determineA(self, xP));
#0 - 0xean
2022-01-08T23:02:29Z
duplicate of #238
20.1994 USDC - $20.20
hyh
Gas overspending due to excessive storage reads
SwapUtils's getVirtualPrice retrieves lptoken handle twice https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L714
Now: uint256 supply = self.lpToken.totalSupply(); ... return d.mul(10uint256(ERC20(self.lpToken).decimals())).div(supply); To be: LPToken memory lp = self.lpToken; uint256 supply = lp.totalSupply(); ... return d.mul(10uint256(ERC20(lp).decimals())).div(supply);
#0 - 0xean
2022-01-08T23:05:13Z
dupe of #242
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas overspending due to excessive storage reads
SwapUtils's getVirtualPrice repetitively calls _xp(self), which reads storage https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L705
Now: uint256 a = determineA(self, _xp(self)); uint256 d = getD(_xp(self), a);
To be: uint256[] memory xP = _xp(self.balances, self.tokenPrecisionMultipliers); uint256 d = getD(xP, determineA(self, xP));
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas overspending due to excessive external function calls
SwapUtils's addLiquidity function calls LP token totalSupply() several times: 6 code occurrences, one is in cycle. The very last occurrency should be kept as it is, the first 5 of them should be replaced with memory variable as the supply changes only once when LP mint() is called at the end of the function. https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1163
Code update:
Now: if (self.lpToken.totalSupply() != 0) { ...
To be: uint256 lpTotalSupply = self.lpToken.totalSupply(); // storage read and function call if (lpTotalSupply != 0) { ...
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas overspending due to excessive operations
SwapUtils.calculateTokenAmount's 'deposit' bool variable is checked on each iteration, while one check is enough https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1031
It's recommended to separate the cycles:
Now: for (uint256 i = 0; i < numTokens; i++) { if (deposit) { balances1[i] = balances1[i].add(amounts[i]); } else { balances1[i] = balances1[i].sub( amounts[i], "Cannot withdraw more than available" ); } }
To be: if (deposit) { for (uint256 i = 0; i < numTokens; i++) { balances1[i] = balances1[i].add(amounts[i]); } } else { for (uint256 i = 0; i < numTokens; i++) { balances1[i] = balances1[i].sub( amounts[i], "Cannot withdraw more than available" ); } }
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas overspending due to excessive operations
getD, getY, getYD functions calculate mul(d).div(xp[i].mul(numTokens) within the token cycles https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L538 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L588 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L861
d, numTokens are constant there, so the divisions are redundant.
Introduce (d / numTokens) variable and simplify the multiplication
Now: uint256 c = d; ... for (uint256 i = 0; i < numTokens; i++) { if (i != tokenIndex) { s = s.add(xp[i]); c = c.mul(d).div(xp[i].mul(numTokens));
To be: uint256 c = d; uint256 d_num = d.div(numTokens); ... for (uint256 i = 0; i < numTokens; i++) { if (i != tokenIndex) { s = s.add(xp[i]); c = c.mul(d_num).div(xp[i]);
hyh
AirdropDistribution's functions: updateEmission, available_supply are public, but are not called within the contract (both have internal versions) and can be made external to reduce calling costs.
#0 - chickenpie347
2022-01-03T23:35:40Z
Duplicate of #121
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas is overspent due to excessive storage reads
SwapUtils's swap: saving self.pooledTokens[tokenIndexFrom], which do not change, to memory and reusing will reduce gas costs. https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1098
Now: self.pooledTokens[tokenIndexFrom].balanceOf(msg.sender)... ... uint256 beforeBalance = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)); self.pooledTokens[tokenIndexFrom].safeTransferFrom( ... uint256 transferredDx = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)).sub(beforeBalance); To be: IERC20 memory fromToken = self.pooledTokens[tokenIndexFrom]; fromToken.balanceOf(msg.sender)... ... uint256 beforeBalance = fromToken.balanceOf(address(this)); fromToken.safeTransferFrom( ... uint256 transferredDx = fromToken.balanceOf(address(this)).sub(beforeBalance);
🌟 Selected for report: hyh
44.8876 USDC - $44.89
hyh
Gas overspending due to excessive storage reads and function calls
SwapUtils's removeLiquidityImbalance does multiple _xp(self) calls, which can be saved to memory when balances don't change inbetween executions https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1415
Now: uint256[] memory balances1 = self.balances; v.preciseA = determineA(self, _xp(self)); v.d0 = getD(_xp(self), v.preciseA); ... v.d1 = getD(_xp(self, balances1), determineA(self, _xp(self, balances1))); ... v.d2 = getD(_xp(self, balances1), determineA(self, _xp(self, balances1)));
To be: uint256[] memory balances1 = self.balances; uint256[] memory tokenPM = self.tokenPrecisionMultipliers; // doesn't change, save and reuse uint256[] memory xP = _xp(balances1, tokenPM); // We already copied self.balances, no need to reread storage v.d0 = getD(xP, determineA(self, xP)); // v.preciseA isn't used elsewhere and can be dropped ... xP = _xp(balances1, tokenPM); // balances1 was modified, recomputing v.d1 = getD(xP, determineA(self, xP)); ... xP = _xp(balances1, tokenPM); // balances1 was modified, recomputing v.d2 = getD(xP, determineA(self, xP));
hyh
Gas overspending due to excessive external storage reads
SwapUtils's calculateWithdrawOneTokenDY, _calculateRemoveLiquidity, _feePerToken, addLiquidity, removeLiquidityImbalance functions read self.pooledTokens.length storage variable up to 2-5 times. This length doesn't change across the logic of the functions.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L406 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L953 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1080 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1163 https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1415
In all the cases: save array length to memory and reuse this constant value.
Now (addLiquidity example): require(minAmounts.length == self.pooledTokens.length), ...
To be: uint pooledTokensLength = self.pooledTokens.length; // storage read, Swap structure require(minAmounts.length == pooledTokensLength), ...
#0 - chickenpie347
2022-01-03T23:46:33Z
Duplicate of #14