Platform: Code4rena
Start Date: 14/10/2021
Pot Size: $50,000 USDC
Total HM: 3
Participants: 14
Period: 7 days
Judge: 0xean
Total Solo HM: 3
Id: 37
League: ETH
Rank: 2/14
Findings: 4
Award: $14,977.57
🌟 Selected for report: 5
🚀 Solo Findings: 2
🌟 Selected for report: cmichel
6818.1818 USDC - $6,818.18
cmichel
There's a griefing attack where an attacker can make any user transaction for TempusController.exitTempusAMM fail.
In _exitTempusAMM, the user exits their LP position and claims back yield and principal shares.
The LP amounts to redeem are determined by the function parameter lpTokensAmount.
A final assert(tempusAMM.balanceOf(address(this)) == 0) statement checks that the LP token amount of the contract is zero after the exit.
This is only true if no other LP shares were already in the contract.
However, an attacker can frontrun this call and send the smallest unit of LP shares to the contract which then makes the original deposit-and-fix transaction fail.
All exitTempusAMM calls can be made to fail and this function becomes unusable.
Remove the assert check.
#0 - mijovic
2021-10-20T19:15:42Z
Great finding. This can block people exiting AMM via TempusController.
#1 - mijovic
2021-10-21T07:50:52Z
🌟 Selected for report: cmichel
6818.1818 USDC - $6,818.18
cmichel
There's a griefing attack where an attacker can make any user transaction for TempusController.depositAndFix fail.
In _depositAndFix, swapAmount many yield shares are swapped to principal where swapAmount is derived from the function arguments.
A final assert(yieldShares.balanceOf(address(this)) == 0) statement checks that the yield shares of the contract are zero after the swap.
This is only true if no other yield shares were already in the contract.
However, an attacker can frontrun this call and send the smallest unit of yield shares to the contract which then makes the original deposit-and-fix transaction fail.
All depositAndFix calls can be made to fail and this function becomes unusable.
Remove the assert check.
#0 - mijovic
2021-10-20T20:36:48Z
Good catch. This can block users from doing this action via controller
#1 - mijovic
2021-10-21T07:50:21Z
1022.7273 USDC - $1,022.73
cmichel
The (second) TempusController._exitTempusAmmAndRedeem function swaps the difference of yield and principal shares using the AMM.
swap( tempusAMM, tempusAMM.getSwapAmountToEndWithEqualShares(principals, yields, maxLeftoverShares), tokenIn, tokenOut, 0 // @audit min return of zero ); // yields and principals are updated to the received amounts and redeemed // ...
It does not use a min return value for this swap and it is, therefore, susceptible to sandwich attacks.
A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.
Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the user's loss.
Add minimum return amount checks.
Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.
Similar to minLpAmountsOut but for the yields & principal shares (or the redeemed tokens).
#0 - mijovic
2021-10-20T19:04:36Z
Very good finding. We are aware of this, and we are adding checks for both minReturn and deadline for TempusControleer functions that include swaps.
#1 - mijovic
2021-10-26T10:40:06Z
🌟 Selected for report: cmichel
159.2357 USDC - $159.24
cmichel
In CompoundTempusPool, the cToken and the base class' yieldBearingToken storage fields are the same.
Remove the cToken field and the assignment in the constructor to save gas.
#0 - mijovic
2021-10-20T18:54:09Z
None of this is storage variable, both are immutables. However, they are duplicated and we should remove the one from CompoundTempusPool
Fixed in https://github.com/tempus-finance/tempus-protocol/pull/375
🌟 Selected for report: cmichel
159.2357 USDC - $159.24
cmichel
The ERC20OwnerMintableToken.burn function loads the manager storage field for the _burn(manager, amount) field.
One can save gas by using _burn(msg.sender, amount) as it was already verified that msg.sender == manager.
#0 - mijovic
2021-10-20T18:44:46Z
This is a very minor and completely irrelevant gas saving.
Have in mind that manager is immutable, so it is not read from storage. The compiler uses codecopy for immutable, so it is very cheap.
#1 - 0xean
2021-10-28T17:42:50Z
there is a gas savings here, albeit small.
21328 vs 21261 for calling test2 vs test3 in the below snippet ran using remix
// SPDX-License-Identifier: GPL-3.0 pragma solidity >=0.7.0 <0.9.0; contract Manager { address public immutable manager; constructor() { manager = msg.sender; } function test2() public { require(msg.sender == manager); fake(manager); } function test3() public { require(msg.sender == manager); fake(msg.sender); } function fake(address test) public { address next = test; } }
#2 - mijovic
2021-10-28T22:07:31Z
there is a gas savings here, albeit small.
21328 vs 21261 for calling test2 vs test3 in the below snippet ran using remix
// SPDX-License-Identifier: GPL-3.0 pragma solidity >=0.7.0 <0.9.0; contract Manager { address public immutable manager; constructor() { manager = msg.sender; } function test2() public { require(msg.sender == manager); fake(manager); } function test3() public { require(msg.sender == manager); fake(msg.sender); } function fake(address test) public { address next = test; } }
In that case we should say this is gas-saving, but we will probably not solve it at this particular time