Platform: Code4rena
Start Date: 29/07/2021
Pot Size: $20,000 USDC
Total HM: 8
Participants: 12
Period: 3 days
Judge: LSDan
Total Solo HM: 2
Id: 24
League: ETH
Rank: 1/12
Findings: 6
Award: $4,899.69
🌟 Selected for report: 7
🚀 Solo Findings: 1
🌟 Selected for report: GalloDaSballo
838.3068 USDC - $838.31
0xRajeev
SwappableYieldSource.sol has a transferERC20() function callable only by the owner or asset manager to transfer out ERC20 tokens other than the yield source's tokens held by this contract. This is similar to the functions in ATokenYieldSource and IdleYieldSource.
In ATokenYieldSource and IdleYieldSource, the token exclusion check is made on aToken and idleToken respectively because they are specific to those yield contracts.
However, in this generic SwappableYieldSource contract which points to the chosen yield source, there is not one permanent yield source token. So the exclusion check has to be performed on the token of the current yield source. But the check implemented checks against the address of the yieldSource contract instead of its token. This will pass even for the yieldSource token and will allow owner or asset manager to transfer out at any time all the yieldSource tokens to any recipient address of choice i.e. rugging.
For e.g. if SwappableYieldSource yieldSource points to ATokenYieldSource, the check will allow aToken to be transferred out because it checks the ERC20 aToken address against ATokenYieldSource address which will pass and allow the transfer out.
Manual Analysis
Check against yield source token address instead of yield source address.
#0 - PierrickGT
2021-08-11T23:17:08Z
This exploit is not possible.
The swappable yield source don't old any funds, this is the role of the yield source that either mint ERC20 token shares to the swappable yield source or keep track of the balance by using a mapping.
That's why we only check that transferERC20 can't move ERC20 token shares that would represent the swappable yield source shares in the yield source.
#1 - 0xean
2021-08-24T16:35:18Z
I agree that there is a potential for a rug pull here, it would have to be in combination with #14 but the potential exists. I am marking as a duplicate as #14 as the issues are related and the check here does little to avoid tokens being transferred after the yieldSource has been changed.
#2 - PierrickGT
2021-08-30T15:33:44Z
Closing this issue that has been marked as a duplicate.
1397.178 USDC - $1,397.18
0xRajeev
Similar to the check in _setYieldSource(), it is safer to check in transferFunds() that the deposit token of the yield source being transferred from is the same as the current yield source.
An owner or assetManager can mistakenly initiate transfer from a yield source with a different depositToken, e.g. DAI, to the current one, e.g. mUSD.
Manual Analysis
Add the check similar to the one in L256 to transferFunds().
#0 - PierrickGT
2021-08-06T16:36:38Z
🌟 Selected for report: 0xRajeev
931.452 USDC - $931.45
0xRajeev
The SwappableYieldSource allows owners and asset managers to set/swap/transfer yield sources/funds. As such, the contract ownership plays a critical role in the protocol.
Given that AssetManager is derived from Ownable, the ownership management of this contract defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes.
Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() or onlyOwnerOrAssetManager() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
Manual Analysis
Override the inherited methods to null functions and use separate functions for a two-step address change: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.
#0 - PierrickGT
2021-08-11T23:05:02Z
This isn't a security issue but an improper use of the initialize function.
We do check for address zero so at least the risk of deploying the contract with address zero is excluded. Also, these contracts will be deployed by a multi sig owned by governance so the risk of a single human error is almost null.
#1 - 0xean
2021-08-24T16:41:58Z
Disagree with sponsor. A two step process would be a safer implementation. A multi-sig does not remove human error or the potential risk here. It may be an acceptable risk to the team, but still worth highlighting with the given severity.
#2 - PierrickGT
2021-08-30T23:38:14Z
We have studied this solution and decided to not implement it since it would make it a pretty tedious process to deploy a swappable yield source, especially through the use of our builder which would mean that a user would have to manually claimOwnership after deploying a pool. Plus, this contract will be owned by governance so it will be very difficult to transfer it to another owner or renounce ownership.
169.7571 USDC - $169.76
0xRajeev
Unlike SwappableYieldSource which uses safeIncreaseAllowance to increase the allowance to uint256.max, mStableYieldSource uses OpenZeppelin’s safeApprove() which has been documented as 1) Deprecated because of approve-like race condition and 2) To be used only for initial setting of allowance (current allowance == 0) or resetting to 0 because it reverts otherwise.
The usage here is intended to allow increase of allowance when it falls low similar to the documented usage in SwappableYieldSource. Using it for that scenario will not work as expected because it will always revert if current allowance is != 0. The initial allowance is already set as uint256.max in constructor. And once it gets reduced, it can never be increased using this function unless it is invoked when allowance is reduced completely to 0.
Manual Analysis
Use logic similar to SwappableYieldSource instead of using safeApprove().
#0 - PierrickGT
2021-08-13T21:34:02Z
This issue has been fixed in the following commit: https://github.com/pooltogether/pooltogether-mstable/pull/3/commits/156a990901e6ddff543897905e3ea3d09c78d817
139.7178 USDC - $139.72
0xRajeev
The SwappableYieldSource.sol has a public visibility initialization function that can be front-run, allowing an attacker to incorrectly initialize the contract, if the deployment of this contract does not safely handle initializations via a robust deployment script or a factory contract to prevent front-running.
Impact: Initialization function can be front-run by attackers, allowing them to initialize the contract with malicious values. Also, if initializations are not done atomically with creation, all public/external functions can be accessed before initialization because there are no checks to confirm initializations in those functions.
Reference: See similar High-severity Finding 9 of Trail of Bits audit of Advanced Blockchain: https://github.com/trailofbits/publications/blob/master/reviews/AdvancedBlockchain.pdf and Finding 12 from Trail of Bits audit of Hermez Network https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
Manual Analysis
Ensure atomic creation+deployment with script or factory contract. Add checks to confirm initialization in public/external functions.
#0 - PierrickGT
2021-08-06T16:24:37Z
This is why we use the freeze function in our deployment: https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/deploy/deploy.ts#L71
#1 - 0xean
2021-08-24T16:54:10Z
The freeze function is not atomic with the deployment and the script does not enforce that that call is made before moving on to further deployments. The script could enforce that the contract has not been initialized which would at least somewhat mitigate the impacts of a potential front run.
#2 - PierrickGT
2021-08-31T01:19:16Z
We are using a factory contract to deploy the Swappable Yield Source so there is no risk of front running: https://github.com/pooltogether/swappable-yield-source/blob/main/deploy/deploy.ts#L92
🌟 Selected for report: 0xRajeev
310.484 USDC - $310.48
0xRajeev
Zero-address checks as input validation closest to the function beginning is a best-practice. There are two places where an explicit zero-address check is missing which may lead to a later revert, gas wastage or even token burn.
Manual Analysis
Add explicit zero-address checks closest to the function entry.
#0 - PierrickGT
2021-08-12T22:44:17Z
Swappable Yield Source PR: https://github.com/pooltogether/swappable-yield-source/pull/13
🌟 Selected for report: 0xRajeev
310.484 USDC - $310.48
0xRajeev
approveMaxAmount() is onlyOwner while all other privileged functions use onlyOwnerOrAssetManager. This modifier should also be onlyOwnerOrAssetManager to prevent situations where owner has added asset managers and renounced ownership which will prevent accessing this approval function thereafter.
onlyOwnerOrAssetManager: https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L268
Manual Analysis
Change onlyOwner to onlyOwnerOrAssetManager
#0 - PierrickGT
2021-08-11T20:46:18Z
We have decided to only allow the owner to run approveMaxAmount for an added layer of security.
For Swappable Yield Sources handled by PoolTogether governance, a multi sig will be used to ensure that not a single person has control of it, this way we limit the risk of the owner renouncing ownership.
#1 - 0xean
2021-08-24T17:01:10Z
The added security here is dubious given the privileges the asset manager currently has. Would recommend rethinking this approach.
#2 - PierrickGT
2021-08-30T22:59:02Z
After discussing with the team, we have decided to make approveMaxAmount public in the following commit, since this emergency function should only be called in the case the allowance would have dropped too low.
https://github.com/pooltogether/swappable-yield-source/pull/9/commits/18e66ae53279d4ef008e271f57fd82500261823f
Also, we have decided to only allow funds to me moved to the current yield source, so this function shouldn't be used by a malicious actor to steal funds. The changes have been made in the following PR: https://github.com/pooltogether/swappable-yield-source/pull/15
🌟 Selected for report: hickuphh3
Also found by: 0xRajeev, GalloDaSballo, cmichel, pauliax
26.3992 USDC - $26.40
0xRajeev
In _setYieldSource() a call to _newYieldSource.depositToken() is made twice. Caching the return value in a local variable can prevent an extra CALL saving 2600 gas.
Manual Analysis
Cache return value of _newYieldSource.depositToken() in a local variable and use that on L259.
#0 - PierrickGT
2021-08-06T16:30:36Z
🌟 Selected for report: maplesyrup
26.3992 USDC - $26.40
0xRajeev
Assuming the initialize() function is going to be called from a deployment script, its visibility can be made external.
For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external function’s parameters are not copied into memory and are instead read from calldata directly.
Manual Analysis
Change visibility to external.
#0 - PierrickGT
2021-08-12T16:42:11Z
🌟 Selected for report: 0xRajeev
201.183 USDC - $201.18
0xRajeev
The zero-address check on owner is present even in transferOwnership() which makes it redundant.
Manual Analysis
Remove explicit check to rely on the one in transferOwnership().
#0 - PierrickGT
2021-08-12T16:53:47Z
🌟 Selected for report: 0xRajeev
201.183 USDC - $201.18
0xRajeev
In redeemToken(), given that mAssetBalanceAfter will always be >= mAssetBalanceBefore, using the unchecked directive (solc 0.8.2 has default overflow/underflow checks) on L106 can save bit of gas from the unnecessary (in this case) internal underflow checks on the subtraction.
Manual Analysis
Change to unchecked {mAssetsActual = mAssetBalanceAfter - mAssetBalanceBefore;}
#0 - PierrickGT
2021-08-16T21:34:39Z
36.6656 USDC - $36.67
0xRajeev
In approveMax(), instead of making an external call to savings.underlying() getter (costs 2600 gas), we can use the immutable mAsset from the contract which was initialized with this value in the constructor. This will avoid the external call and save 2600 gas.
Manual Analysis
Replace use of savings.underlying() with mAsset.
#0 - PierrickGT
2021-08-13T16:10:03Z