Platform: Code4rena
Start Date: 17/06/2021
Pot Size: $60,000 USDC
Total HM: 12
Participants: 12
Period: 7 days
Judge: LSDan
Total Solo HM: 8
Id: 14
League: ETH
Rank: 4/12
Findings: 6
Award: $11,074.45
🌟 Selected for report: 10
🚀 Solo Findings: 2
🌟 Selected for report: shw
5827.0101 USDC - $5,827.01
shw
The redeemToken function in IdleYieldSource uses redeemedShare instead of redeemAmount as the input parameter when calling redeemIdleToken of the Idle yield source. As a result, users could get fewer underlying tokens than they should.
When burning users' shares, it is correct to use redeemedShare (line 130). However, when redeeming underlying tokens from Idle Finance, redeemAmount should be used instead of redeemedShare (line 131). Usually, the tokenPriceWithFee() is greater than ONE_IDLE_TOKEN, and thus redeemedShare is less than redeemAmount, causing users to get fewer underlying tokens than expected.
Referenced code: IdleYieldSource.sol#L129-L131
Change redeemedShare to redeemAmount at line 131.
#0 - PierrickGT
2021-06-30T10:29:24Z
229.3861 USDC - $229.39
shw
In the contracts BadgerYieldSource and SushiYieldSource, the return values of ERC20 transfer and transferFrom are not checked to be true, which could be false if the transferred tokens are not ERC20-compliant (e.g., BADGER). In that case, the transfer fails without being noticed by the calling contract.
If warden's understanding of the BadgerYieldSource is correct, the badger variable should be the BADGER token at address 0x3472a5a71965499acd81997a54bba8d852c6e53d. However, this implementation of BADGER is not ERC20-compliant, which returns false when the sender does not have enough token to transfer (both for transfer and transferFrom). See the source code on Etherscan (at line 226) for more details.
Referenced code: BadgerYieldSource.sol#L44 BadgerYieldSource.sol#L79 SushiYieldSource.sol#L48 SushiYieldSource.sol#L89
Use the SafeERC20 library implementation from Openzeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.
#1 - dmvt
2021-07-31T21:05:36Z
Sponsor has repeatedly stated in duplicate issues that: "It's more of a 1 (Low Risk) because the subsequent deposit calls will fail. There is no advantage to be gained; the logic is simply poor."
I disagree with this assessment. The function(s) in question do not immediately call deposit or another function that would cause a revert. In fact the balances are updated:
balances[msg.sender] = balances[msg.sender].sub(requiredSharesBalance); badger.transfer(msg.sender, badgerBalanceDiff); return (badgerBalanceDiff);
The impact that this would have on the rest of the system is substantial, including causing incorrect balances to be returned and potentially lost funds.
That said, I do not think this is very likely and so high severity seems excessive here. Im adjusting all of these reports to Medium Risk given that lower likelihood.
786.6464 USDC - $786.65
shw
SafeMath is not completely used at the following lines of yield source contracts, which could potentially cause arithmetic underflow and overflow:
SushiYieldSourceBadgerYieldSourceIdleYieldSourceReferenced code: SushiYieldSource.sol#L78 BadgerYieldSource.sol#L67 IdleYieldSource.sol#L91 IdleYieldSource.sol#L98
Use the SafeMath library functions in the above lines.
#0 - asselstine
2021-06-24T23:21:04Z
While the arithmetic ceiling is quite high, if an overflow occurred this would significantly disrupt the yield sources. I'd qualify this issue higher as 2 (Med Risk).
#1 - dmvt
2021-08-24T13:32:48Z
I agree with the sponsor's risk evaluation. Increasing to medium.
🌟 Selected for report: shw
1748.103 USDC - $1,748.10
shw
In the function awardExternalERC721 of contract PrizePool, when awarding external ERC721 tokens to the winners, the transferFrom keyword is used instead of safeTransferFrom. If any winner is a contract and is not aware of incoming ERC721 tokens, the sent tokens could be locked.
Referenced code: PrizePool.sol#L602
Consider changing transferFrom to safeTransferFrom at line 602. However, it could introduce a DoS attack vector if any winner maliciously rejects the received ERC721 tokens to make the others unable to get their awards. Possible mitigations are to use a try/catch statement to handle error cases separately or provide a function for the pool owner to remove malicious winners manually if this happens.
#0 - asselstine
2021-06-24T23:18:22Z
This issue poses no risk to the Prize Pool, so it's more of a 1 (Low Risk IMO.
This is just about triggering a callback on the ERC721 recipient. We omitted it originally because we didn't want a revert on the callback to DoS the prize pool.
However, to respect the interface it makes sense to implement it fully. That being said, if it does throw we must ignore it to prevent DoS attacks.
#1 - dmvt
2021-08-27T22:27:32Z
I agree with the medium risk rating provided by the warden.
106.1973 USDC - $106.20
shw
The YearnV2YieldSource contract prevents the supplyTokenTo, redeemToken, and sponsor functions from being reentered by applying a nonReentrant modifier. Since these contracts share a similar logic, adding a nonReentrant modifier to these functions in all of the yield source contracts is reasonable. However, the same protection is not seen in other yield source contracts.
A nonReentrant modifier in the following functions is missing:
sponsor function of ATokenYieldSourcesupplyTokenTo and redeemToken function of BadgerYieldSourcesponsor function of IdleYieldSourcesupplyTokenTo and redeemToken function of SushiYieldSourceReferenced code: ATokenYieldSource.sol#L233 BadgerYieldSource.sol#L43 BadgerYieldSource.sol#L57 IdleYieldSource.sol#L150 SushiYieldSource.sol#L47 SushiYieldSource.sol#L66
Add a nonReentrant modifier to these functions. For BadgerYieldSource and SushiYieldSource contracts, make them inherit from Openzeppelin's ReentrancyGuardUpgradeable to use the nonReentrant modifier.
#0 - kamescg
2021-06-28T19:35:56Z
ATokenYieldSource: https://github.com/pooltogether/aave-yield-source/tree/fix/119 SushiYieldSource: https://github.com/pooltogether/sushi-pooltogether/pull/new/fix/119 BadgerYieldSource: https://github.com/pooltogether/badger-yield-source/pull/new/fix/119 IdleYieldSource: https://github.com/pooltogether/idle-yield-source/pull/new/fix/119
shw
The initialize function in IdleYieldSource and YearnV2YieldSource does not include a __ERC20_init function call such as that in ATokenYieldSource. As a result, the two ERC20 tokens would have empty string as their names or symbols.
Referenced code: IdleYieldSource.sol#L56-L67 YearnV2YieldSource.sol#L66-L93
Provide parameters name and symbol to initialize the ERC20 tokens using __ERC20_init(name, symbol).
#0 - asselstine
2021-06-24T23:39:50Z
#1 - dmvt
2021-08-24T20:50:23Z
duplicate of #60
157.3293 USDC - $157.33
shw
Some contracts (e.g., PrizePool) use an unlocked pragma (e.g., pragma solidity >=0.6.0 <0.7.0;) which is not fixed to a specific Solidity version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most.
Referenced code:
Please use grep -R pragma . to find the unlocked pragma statements.
Lock pragmas to a specific Solidity version. Consider the compiler bugs in the following lists and ensure the contracts are not affected by them. It is also recommended to use the latest version of Solidity when deploying contracts (see Solidity docs).
Solidity compiler bugs: Solidity repo - known bugs Solidity repo - bugs by version
#0 - kamescg
2021-06-29T23:52:21Z
🌟 Selected for report: shw
582.701 USDC - $582.70
shw
The PrizePool contract does not implement the onERC721Received function, which is considered a best practice to transfer ERC721 tokens from contracts to contracts. The absence of this function could prevent PrizePool from receiving ERC721 tokens from other contracts via safeTransferFrom.
Referenced code: PrizePool.sol
Consider adding an implementation of the onERC721Received function in PrizePool.
54.8467 USDC - $54.85
shw
The redeemToken function in BadgerYieldSource can be designed as more gas-efficient by reusing local variables.
Variables badgerSett and badger in the BadgerYieldSource are currently declared private. However, a similar implementation, SushiYieldSource, declares sushiBar and sushiAddr public. Suppose the badgerSett and badger in BadgerYieldSource are needed to be declared public. In that case, the redeemToken function can be more gas-efficient by reading these two state variables into local variables first and then reusing them, as written in the redeemToken function of SushiYieldSource contract.
Referenced code: BadgerYieldSource.sol#L15-L16 BadgerYieldSource.sol#L57-L81 SushiYieldSource.sol#L17-L18 SushiYieldSource.sol#L66-L92
Let badgerSett and badger be public and re-write the redeemToken function in the BadgerYieldSource contract.
#0 - asselstine
2021-06-24T23:52:04Z
135.4239 USDC - $135.42
shw
Functions (e.g., supplyTokenTo, redeemToken) in the BadgerYieldSource and SushiYieldSource can be declared external instead of public to save gas.
Referenced code: BadgerYieldSource.sol#L26 BadgerYieldSource.sol#L32 BadgerYieldSource.sol#L43 BadgerYieldSource.sol#L57 SushiYieldSource.sol#L29 SushiYieldSource.sol#L35 SushiYieldSource.sol#L47 SushiYieldSource.sol#L66
Change the keyword public to external.
🌟 Selected for report: shw
300.9419 USDC - $300.94
shw
The function _depositToAave of ATokenYieldSource calls _lendingPool and _tokenAddress twice, both of which include function calls to external contracts. Thus, storing the first results into local variables and reuse them for the second time could help save gas.
Referenced code: ATokenYieldSource.sol#L175-L182
Store the result of _tokenAddress() and _lendingPool() to local variables and resue them.
#0 - PierrickGT
2021-06-28T09:26:43Z
🌟 Selected for report: shw
300.9419 USDC - $300.94
shw
At line 213 of ATokenYieldSource, depositToken() can be replaced by _tokenAddress() to save gas since the former is a public function, while the latter is an internal function.
Referenced code: ATokenYieldSource.sol#L213
Change depositToken() to _tokenAddress().
#0 - PierrickGT
2021-06-28T09:04:53Z