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: 6/12
Findings: 3
Award: $1,856.78
🌟 Selected for report: 3
🚀 Solo Findings: 0
gpersoon
3 of the 5 yield sources use safeTransfer/safeTransferFrom (ATokenYieldSource.sol, IdleYieldSource.sol, YearnV2YieldSource.sol) However 2 of the 5 yielsd source use transfer/transferFrom and also don't check the result of the functions (BadgerYieldSource.sol, SushiYieldSource.sol)
This way you are dependant on the fact that transfer/transferFrom reverts on an error (insufficient funds), as the ERC20 contracts are know this will be the case. However its safer to use safeTransfer/safeTransferFrom or check the result of the functions transfer/transferFrom. Especially if these contracts would be used as a template to create yield sources for other ERC20 contracts.
.\contracts\yield-source\ATokenYieldSource.sol: _depositToken.safeTransferFrom(msg.sender, address(this), mintAmount); .\contracts\yield-source\ATokenYieldSource.sol: IERC20Upgradeable(depositToken()).safeTransfer(msg.sender, balanceDiff); .\contracts\yield-source\IdleYieldSource.sol: IERC20Upgradeable(underlyingAsset).safeTransferFrom(msg.sender, address(this), mintAmount); .\contracts\yield-source\IdleYieldSource.sol: IERC20Upgradeable(underlyingAsset).safeTransfer(msg.sender, redeemedUnderlyingAsset); .\contracts\yield-source\YearnV2YieldSource.sol: token.safeTransferFrom(msg.sender, address(this), _amount); .\contracts\yield-source\YearnV2YieldSource.sol: token.safeTransfer(msg.sender, withdrawnAmount); .\contracts\yield-source\YearnV2YieldSource.sol: token.safeTransferFrom(msg.sender, address(this), amount);
.\contracts\yield-source\BadgerYieldSource.sol: badger.transferFrom(msg.sender, address(this), amount); .\contracts\yield-source\BadgerYieldSource.sol: badger.transfer(msg.sender, badgerBalanceDiff); .\contracts\yield-source\SushiYieldSource.sol: sushiAddr.transferFrom(msg.sender, address(this), amount); .\contracts\yield-source\SushiYieldSource.sol: sushi.transfer(msg.sender, sushiBalanceDiff);
Use safeTransfer/safeTransferFrom or check the result of the functions transfer/transferFrom in BadgerYieldSource.sol and SushiYieldSource.sol
#0 - asselstine
2021-06-24T23:29:49Z
#1 - dmvt
2021-07-31T21:06:43Z
duplicate of #112
🌟 Selected for report: gpersoon
gpersoon
The initialize function "initializeYieldSourcePrizePool" of YieldSourcePrizePool.sol has a check to make sure _yieldSource !=0 However the initialize function "initialize" of the comparable StakePrizePool.sol doesn't do this check.
Although unlikely this will introduce problems, it is more consistent to check for 0.
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/YieldSourcePrizePool.sol#L24 function initializeYieldSourcePrizePool (... IYieldSource _yieldSource) ... { .. require(address(_yieldSource) != address(0), "YieldSourcePrizePool/yield-source-zero"); PrizePool.initialize( ...
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/StakePrizePool.sol#L20 function initialize ( .. IERC20Upgradeable _stakeToken)... { PrizePool.initialize( ...
Add something like the following in the initialize function of StakePrizePool.sol: require(address(_stakeToken) != address(0), "StakePrizePool/stakeToken-zero");
gpersoon
The functions supplyTokenTo and redeemToken have an extra secury measure of nonReentrant in 3 of the 5 yield source contracts: ATokenYieldSource.sol, IdleYieldSource.sol, YearnV2YieldSource.sol However in the following 2 yield source contracts its not present: BadgerYieldSource.sol, SushiYieldSource.sol
Although I don't see any immediate issue, it can't hurt to add nonReentrant to prevent potential reentrancy problems.
.\contracts\yield-source\ATokenYieldSource.sol: function supplyTokenTo(uint256 mintAmount, address to) external override nonReentrant { .\contracts\yield-source\IdleYieldSource.sol: function supplyTokenTo(uint256 mintAmount, address to) external nonReentrant override { .\contracts\yield-source\YearnV2YieldSource.sol: function supplyTokenTo(uint256 _amount, address to) external override c{
.\contracts\yield-source\BadgerYieldSource.sol: function supplyTokenTo(uint256 amount, address to) public override { .\contracts\yield-source\SushiYieldSource.sol: function supplyTokenTo(uint256 amount, address to) public override {
.\contracts\yield-source\ATokenYieldSource.sol: function redeemToken(uint256 redeemAmount) external override nonReentrant returns (uint256) { .\contracts\yield-source\IdleYieldSource.sol: function redeemToken(uint256 redeemAmount) external override nonReentrant returns (uint256 redeemedUnderlyingAsset) { .\contracts\yield-source\YearnV2YieldSource.sol: function redeemToken(uint256 amount) external override nonReentrant returns (uint256) {
.\contracts\yield-source\BadgerYieldSource.sol: function redeemToken(uint256 amount) public override returns (uint256) { .\contracts\yield-source\SushiYieldSource.sol: function redeemToken(uint256 amount) public override returns (uint256) {
grep
Add nonReentrant to the functions supplyTokenTo and redeemToken of BadgerYieldSource.sol and SushiYieldSource.sol
#0 - asselstine
2021-06-24T23:05:31Z
🌟 Selected for report: gpersoon
gpersoon
In PrizePool.sol, if the value of _tokenCreditPlans[_controlledToken].creditRateMantissa isn't set (yet), then the function _estimateCreditAccrualTime returns 0. This means the TimelockDuration is 0 and funds can be withdrawn immediately, defeating the entire timelock mechanism.
Perhaps a different default would be useful.
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/PrizePool.sol#L783 function _estimateCreditAccrualTime( address _controlledToken,uint256 _principal,uint256 _interest ) internal view returns (uint256 durationSeconds) { uint256 accruedPerSecond = FixedPoint.multiplyUintByMantissa(_principal, _tokenCreditPlans[_controlledToken].creditRateMantissa); if (accruedPerSecond == 0) { return 0; } return _interest.div(accruedPerSecond); }
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/PrizePool.sol#L710 function _calculateTimelockDuration( address from, address controlledToken, uint256 amount) internal returns (uint256 durationSeconds, uint256 burnedCredit ) { ... uint256 duration = _estimateCreditAccrualTime(controlledToken, amount, exitFee); if (duration > maxTimelockDuration) { duration = maxTimelockDuration; } return (duration, _burnedCredit); }
Consider the default duration for the case _tokenCreditPlans[_controlledToken].creditRateMantissa isn't set.
54.8467 USDC - $54.85
gpersoon
In the function redeemToken of SushiYieldSource.sol the variables sushiBar and sushiAddr are cached to safe some gas. In the similar function redeemToken of BadgerYieldSource.sol this isn't done. A bit of gas could be safed using the same mechanism.
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/yield-source/SushiYieldSource.sol#L66 function redeemToken(uint256 amount) public override returns (uint256) { ISushiBar bar = sushiBar; ISushi sushi = sushiAddr; uint256 totalShares = bar.totalSupply(); .. uint256 barSushiBalance = sushi.balanceOf(address(bar)); .. uint256 sushiBeforeBalance = sushi.balanceOf(address(this)); .. bar.leave(requiredSharesBalance); uint256 sushiAfterBalance = sushi.balanceOf(address(this));
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/yield-source/BadgerYieldSource.sol#L57 function redeemToken(uint256 amount) public override returns (uint256) { uint256 totalShares = badgerSett.totalSupply(); ... uint256 badgerSettBadgerBalance = badgerSett.balance(); .. uint256 badgerBeforeBalance = badger.balanceOf(address(this)); .. badgerSett.withdraw(requiredSharesBalance); uint256 badgerAfterBalance = badger.balanceOf(address(this));
In function redeemToken of BadgerYieldSource.sol, use temporary variables like: IBadgerSett tmpbadgerSett = badgerSett; IBadger tmpbadger = badger; And replace badgerSett with tmpbadgerSett and replace badger with tmpbadger .
#0 - asselstine
2021-06-24T23:52:28Z
🌟 Selected for report: gpersoon
gpersoon
In the function _sweepTimelockBalances of PrizePool.sol, the function _currentTime() is called from within a loop. As the _currentTime() doesn't change, it is possible to put it ourside the loop. This is only relevant if _sweepTimelockBalances is regularly called with a relatively large array of users.
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/PrizePool.sol#L639 function _sweepTimelockBalances( address[] memory users ) internal returns (uint256) { .. for (i = 0; i < users.length; i++) { address user = users[i]; if (_unlockTimestamps[user] <= _currentTime()) { ... } }
// https://github.com/code-423n4/2021-06-pooltogether/blob/main/contracts/PrizePool.sol#L1021 function _currentTime() internal virtual view returns (uint256) { return block.timestamp; }
Set a temporary variable with the value of _currentTime() and use the temporary variable in the loop