Platform: Code4rena
Start Date: 13/12/2021
Pot Size: $75,000 USDC
Total HM: 11
Participants: 30
Period: 7 days
Judge: leastwood
Total Solo HM: 4
Id: 68
League: ETH
Rank: 1/30
Findings: 6
Award: $12,278.38
🌟 Selected for report: 13
🚀 Solo Findings: 1
🌟 Selected for report: WatchPug
10014.677 USDC - $10,014.68
WatchPug
Under certain circumstances, e.g. annualizedFee being minted to feeBeneficiary between the time user sent the transaction and the transaction being packed into the block and causing amounts of underlying tokens for each basketToken to decrease. It's possible or even most certainly that there will be some leftover basket underlying tokens, as BasketFacet.sol#joinPool() will only transfer required amounts of basket tokens from Join contracts.
However, in the current implementation, only the leftover inputToken is returned.
As a result, the leftover underlying tokens won't be returned to the user, which constitutes users' fund loss.
function joinTokenSingle(JoinTokenStructV2 calldata _joinTokenStruct) external { // ######## INIT TOKEN ######### IERC20 inputToken = IERC20(_joinTokenStruct.inputToken); inputToken.safeTransferFrom( msg.sender, address(this), _joinTokenStruct.inputAmount ); _joinTokenSingle(_joinTokenStruct); // ######## SEND TOKEN ######### uint256 remainingIntermediateBalance = inputToken.balanceOf( address(this) ); if (remainingIntermediateBalance > 0) { inputToken.safeTransfer(msg.sender, remainingIntermediateBalance); } }
function joinPool(uint256 _amount, uint16 _referral) external override noReentry { require(!this.getLock(), "POOL_LOCKED"); chargeOutstandingAnnualizedFee(); LibBasketStorage.BasketStorage storage bs = LibBasketStorage.basketStorage(); uint256 totalSupply = LibERC20Storage.erc20Storage().totalSupply; require( totalSupply.add(_amount) <= this.getCap(), "MAX_POOL_CAP_REACHED" ); uint256 feeAmount = _amount.mul(bs.entryFee).div(10**18); for (uint256 i; i < bs.tokens.length; i++) { IERC20 token = bs.tokens[i]; uint256 tokenAmount = balance(address(token)).mul(_amount.add(feeAmount)).div( totalSupply ); require(tokenAmount != 0, "AMOUNT_TOO_SMALL"); token.safeTransferFrom(msg.sender, address(this), tokenAmount); } ...
Furthermore, the leftover tokens in the SingleTokenJoinV2 contract can be stolen by calling joinTokenSingle() with fake outputBasket contract and swap.exchange contract.
Consider changing to:
IBasketFacet.calcTokensForAmount() first and only swap for exactly the desired amounts of tokens (like SingleTokenJoin.sol);WatchPug
uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" );
In the current implementation, _joinTokenSingle() requires balanceOf outputToken strictly equal to outputAmount in calldata.
However, since anyone can transfer outputToken to the contract, if there are existing outputToken in the balanceOf the contract, joinTokenSingle() will always fail with "FAILED_OUTPUT_AMOUNT".
An attacker can send 1 wei of outputToken and break joinTokenSingle().
The same problem also applies to:
uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" );
Change to:
uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount >= _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" ); outputToken.safeTransfer(msg.sender, _joinTokenStruct.outputAmount);
#0 - loki-sama
2022-01-03T09:29:09Z
duplicate #81
WatchPug
require( totalSupply.add(_amount) <= this.getCap(), "MAX_POOL_CAP_REACHED" ); uint256 feeAmount = _amount.mul(bs.entryFee).div(10**18);
feeAmount should be considered. Otherwise, the new totalSupply may surpass pool cap.
#0 - loki-sama
2022-01-03T11:29:27Z
Duplicate #283
WatchPug
Calling ERC20.transfer() without handling the returned value is unsafe.
outputToken.transfer(msg.sender, outputTokenBalance);
Consider using OpenZeppelin's SafeERC20 library with safe versions of transfer functions.
#0 - loki-sama
2022-01-04T10:29:53Z
duplicate #232
#1 - 0xleastwood
2022-01-22T04:09:05Z
Duplicate of #192
WatchPug
Since the introduction of transfer(), it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance made sense under the assumption that gas costs wouldn’t change. It's now recommended that transfer() and send() be avoided, as gas costs can and will change and reentrancy guard is more commonly used.
Any smart contract that uses transfer() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.
It's recommended to stop using transfer() and switch to using call() instead.
msg.sender.transfer(intermediateTokenBalance);
Can be changed to:
(bool success, ) = msg.sender.call.value(intermediateTokenBalance)(""); require(success, "ETH transfer failed");
#0 - loki-sama
2022-01-03T12:22:38Z
duplicate #175
🌟 Selected for report: sirhashalot
Also found by: GiveMeTestEther, JMukesh, Ruhum, WatchPug, defsec, robee
WatchPug
There are many functions across the codebase that will perform an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.
Instances include:
token.approve(spender, uint256(-1));
It is usually good to add a require-statement that checks the return value or to use something like safeApprove; unless one is sure the given token reverts in case of a failure.
#0 - loki-sama
2022-01-04T11:16:56Z
duplicate #294
WatchPug
totalSupply.mul(annualizedFee).div(10**18).mul(timePassed).div( 365 days );
Can be changed to:
totalSupply.mul(annualizedFee).mul(timePassed).div( 365 days ).div(10**18);
Otherwise, when totalSupply and annualizedFee are smaller numbers, the resulting number can be less than expected due to precision loss.
#0 - 0xleastwood
2022-01-23T04:47:35Z
Duplicate of #155
WatchPug
There are ERC20 tokens that charge fee for every transfer() or transferFrom().
In the current implementation, BasketFacet.sol##joinPool() and SingleTokenJoin.sol#joinTokenSingle() assumes that the received amount is the same as the transfer amount, and uses it for basketToken minting and token swap.
Consider comparing the before and after balanceOf to get the actual transferred amount.
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
PieFactoryContract.sol#bakePie()
BasketFacet.sol#removeToken()
BasketFacet.sol#getTokens()
CallFacet.sol#removeCaller()
SingleNativeTokenExit.sol#exitEth()
#0 - 0xleastwood
2022-01-23T22:13:51Z
Duplicate of #249
10.2787 USDC - $10.28
WatchPug
s.lockCounter++;
Using ++s.lockCounter is more gas efficient than s.lockCounter++ for storage variable.
#0 - 0xleastwood
2022-01-23T22:15:31Z
Duplicate of #108
21.1495 USDC - $21.15
WatchPug
For the arithmetic operations that will never over/underflow, using SafeMath will cost more gas.
For example:
uint256 timePassed = block.timestamp.sub(lastFeeClaimed);
block.timestamp - lastFeeClaimed will never underflow.
es.balances[_to] = es.balances[_to].add(_amount); es.totalSupply = es.totalSupply.add(_amount);
es.balances[_to] + _amount will not overflow if es.totalSupply.add(_amount) dose not overflow.
es.balances[_from] = es.balances[_from].sub(_amount); es.totalSupply = es.totalSupply.sub(_amount);
es.totalSupply - _amount will not underflow if es.balances[_from].sub(_amount) dose not underflow.
#0 - 0xleastwood
2022-01-23T22:20:58Z
Duplicate of #106
21.1495 USDC - $21.15
WatchPug
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
The advantages of versions 0.8.* over <0.8.0 are:
#0 - 0xleastwood
2022-01-24T09:32:48Z
Duplicate of #106
🌟 Selected for report: WatchPug
WatchPug
To wrap native token, using IWrappedNativeToken.deposit() is more gas efficient than .call{value: msg.value}().
address(INTERMEDIATE_TOKEN).call{value: msg.value}("");
address(INTERMEDIATE_TOKEN).call{value: msg.value}("");
Change to:
IWrappedNativeToken(address(INTERMEDIATE_TOKEN)).deposit{ value: msg.value }();
🌟 Selected for report: WatchPug
WatchPug
The current implementation of ReentryProtection will increase lockCounter by one every time it's used.
modifier noReentry { // Use counter to only write to storage once LibReentryProtectionStorage.RPStorage storage s = LibReentryProtectionStorage.rpStorage(); s.lockCounter++; uint256 lockValue = s.lockCounter; _; require( lockValue == s.lockCounter, "ReentryProtectionFacet.noReentry: reentry detected" ); }
A more gas efficient implementation would be switching between 1, 2.
By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).
Change to:
modifier noReentry { // Use counter to only write to storage once LibReentryProtectionStorage.RPStorage storage s = LibReentryProtectionStorage.rpStorage(); s.lockCounter = 2; _; require( s.lockCounter == 2, "ReentryProtectionFacet.noReentry: reentry detected" ); s.lockCounter = 1; }
🌟 Selected for report: WatchPug
WatchPug
10 ** 18 can be changed to 1e18 to avoid unnecessary arithmetic operation and save gas.
35.2492 USDC - $35.25
WatchPug
Considering that underlying, childChainManager will never change, changing them to immutable variables instead of storages variable can save gas.
#0 - 0xleastwood
2022-01-24T09:30:20Z
Duplicate of #18
WatchPug
uint256 feeAmount = _amount.mul(bs.entryFee).div(10**18); for (uint256 i; i < bs.tokens.length; i++) { IERC20 token = bs.tokens[i]; uint256 tokenAmount = balance(address(token)).mul(_amount.add(feeAmount)).div( totalSupply ); require(tokenAmount != 0, "AMOUNT_TOO_SMALL"); token.safeTransferFrom(msg.sender, address(this), tokenAmount); }
_amount.add(feeAmount) is being recalculated each in the for loop, which is unnecessary.
uint256 feeAmount = _amount.mul(bs.exitFee).div(10**18); for (uint256 i; i < bs.tokens.length; i++) { IERC20 token = bs.tokens[i]; uint256 tokenBalance = balance(address(token)); // redeem less tokens if there is an exit fee uint256 tokenAmount = tokenBalance.mul(_amount.sub(feeAmount)).div(totalSupply); require( tokenBalance.sub(tokenAmount) >= MIN_AMOUNT, "TOKEN_BALANCE_TOO_LOW" ); token.safeTransfer(msg.sender, tokenAmount); }
_amount.sub(feeAmount) is being recalculated each in the for loop, which is unnecessary and gas consuming.
🌟 Selected for report: WatchPug
WatchPug
s.lockCounter++; uint256 lockValue = s.lockCounter;
Can be changed to:
uint256 lockValue = s.lockCounter = s.lockCounter + 1;
🌟 Selected for report: WatchPug
WatchPug
require( msg.sender == LibDiamond.diamondStorage().contractOwner || msg.sender == address(this), "NOT_ALLOWED" );
Can be changed to:
require( msg.sender == address(this) || msg.sender == LibDiamond.diamondStorage().contractOwner, "NOT_ALLOWED" );
When msg.sender != address(this), can exit earlier and avoiding more expensive check of msg.sender == LibDiamond.diamondStorage().contractOwner.
🌟 Selected for report: WatchPug
WatchPug
for (uint256 i = 0; i < _tokens.length; i++) { IERC20 token = IERC20(_tokens[i]); token.safeTransferFrom(msg.sender, address(pie), _amounts[i]); pie.addToken(_tokens[i]); }
tokens[i] at L91 is already cached in the local variable token at L89, reusing the result instead of doing the subscript operation again can save gas.
🌟 Selected for report: WatchPug
78.3316 USDC - $78.33
WatchPug
IERC20 inputToken = IERC20(_joinTokenStruct.inputToken); inputToken.safeTransferFrom( msg.sender, address(this), _joinTokenStruct.inputAmount );
inputToken is unnecessary as it's being used only once. Can be changed to:
IERC20(_joinTokenStruct.inputToken).safeTransferFrom( msg.sender, address(this), _joinTokenStruct.inputAmount );
🌟 Selected for report: WatchPug
WatchPug
path[0] = _joinTokenStruct.inputToken; path[1] = address(INTERMEDIATE_TOKEN); uint256[] memory amountsOut = uniSwapLikeRouter.getAmountsOut( _joinTokenStruct.inputAmount, path ); uniSwapLikeRouter.swapExactTokensForTokens( _joinTokenStruct.inputAmount, amountsOut[amountsOut.length - 1], path, address(this), _joinTokenStruct.deadline );
Getting amountsOut with uniSwapLikeRouter.getAmountsOut() before calling uniSwapLikeRouter.swapExactTokensForTokens() and set minAmountOut as amountsOut[amountsOut.length - 1] is unnecessary as it will always met minAmountOut, use 0 for minAmountOut instead can save gas.
Change to:
uniSwapLikeRouter.swapExactTokensForTokens( _joinTokenStruct.inputAmount, 0, path, address(this), _joinTokenStruct.deadline );
🌟 Selected for report: WatchPug
WatchPug
function joinTokenSingle(JoinTokenStruct calldata _joinTokenStruct) external { // ######## INIT TOKEN ######### IERC20 inputToken = IERC20(_joinTokenStruct.inputToken); inputToken.safeTransferFrom( msg.sender, address(this), _joinTokenStruct.inputAmount ); _joinTokenSingle(_joinTokenStruct); // ######## SEND TOKEN ######### uint256 remainingIntermediateBalance = INTERMEDIATE_TOKEN.balanceOf( address(this) ); if (remainingIntermediateBalance > 0) { INTERMEDIATE_TOKEN.safeTransfer( msg.sender, remainingIntermediateBalance ); } }
SingleTokenJoin#joinTokenSingle() can be changed to:
outputAmount;inputToken needed based on the amount of INTERMEDIATE_TOKEN (the output of above);inputToken required from msg.sender;This way we can avoid unnecessary dust INTERMEDIATE_TOKEN.
🌟 Selected for report: WatchPug
WatchPug
require(!this.getLock(), "POOL_LOCKED");
function getLock() external view override returns (bool) { LibBasketStorage.BasketStorage storage bs = LibBasketStorage.basketStorage(); return bs.lockBlock == 0 || bs.lockBlock >= block.number; }
getLock() is being defined and used as an external function (this.getLock()), which costs more gas than using it as an internal function.
We suggest change it to require(!getLock(), "POOL_LOCKED"); for gas saving.