Platform: Code4rena
Start Date: 16/12/2021
Pot Size: $100,000 USDC
Total HM: 21
Participants: 25
Period: 7 days
Judge: alcueca
Total Solo HM: 12
Id: 66
League: ETH
Rank: 2/25
Findings: 7
Award: $13,963.80
🌟 Selected for report: 5
🚀 Solo Findings: 2
1434.4958 USDC - $1,434.50
jayjonah8
In StabilityPool.sol, the receiveCollateral() function should be called by ActivePool per comments, but anyone can call it passing in _tokens and _amounts args to update stability pool balances.
Manual code review
Allow only the ActivePool to call the receiveCollateral() function: require(msg.sender = address(active pool address), "Can only be called by ActivePool")
#0 - kingyetifinance
2022-01-05T03:34:08Z
@LilYeti: This was also caught by our official auditor, but good catch.
#1 - 0xtruco
2022-01-11T07:36:44Z
Fixed this, #190, #285, already in code https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/StabilityPool.sol#L1144
717.2479 USDC - $717.25
jayjonah8
The README.md makes the point that reentrancy attacks are a cause for concern, but the protocol makes no use of reentrancy guards in any file or in the functions users interact with.
The are no Reentrancy guards in the entire code base.
Manual code review
Make use of the Open Zeppelin ReentrancyGuard.sol
#0 - 0xtruco
2022-01-12T01:12:46Z
Resolved in https://github.com/code-423n4/2021-12-yetifinance/pull/17 Also fixes #185 , #183
#1 - alcueca
2022-01-15T06:46:23Z
Duplicate of #183
🌟 Selected for report: jayjonah8
5312.9473 USDC - $5,312.95
jayjonah8
In WJLP.sol, the wrap() function pulls in _amount base tokens from _from, then stakes them to mint WAssets which it sends to _to. It then updates _rewardOwner's reward tracking such that it now has the right to future yields from the newly minted WAssets. But the function does not make sure that _from and _to are not the same address and failure to make this check in functions with transfer functionality has lead to severe bugs in other protocols since users rewards are updated on such transfers this can be used to manipulate the system.
Manual code review
require(address(_from) != address(_to), "_from and _to cannot be the same")
#0 - kingyetifinance
2022-01-07T06:46:10Z
@LilYeti : Originally disputed since we had different wJLP on our main where this is already removed. part of this functionality is intended but the ability to frontrun the approve / wrap call is where the liability is.
🌟 Selected for report: csanuragjain
430.3487 USDC - $430.35
jayjonah8
In WJLP.sol a user can call the claimReward() function to claim the JOE rewards they are owed. This eventually calls the _safeJoeTransfer() function which will check if the _amount to send is greater than the joeBal of the contract. If the _amount is greater than the contract balance, the joeBal of the contract is sent to the user instead. This would mean that they are receiving less JOE than they should since their unclaimedJOEReward is not updated again to refund them the difference of the _amount and joeBal
Manual code review
In the _safeJoeTransfer() function, If the joeBal of the contract is less than the amount owed to the user, then the users unclaimedJOEReward should be updated to reflect this refunding them the difference between the joeBal and _amount
#0 - kingyetifinance
2022-01-03T06:33:41Z
Bug which would produce some dust lost, probably severity 1.
#1 - kingyetifinance
2022-01-05T06:13:39Z
#109 has better description
#2 - kingyetifinance
2022-01-07T07:30:14Z
@LilYeti : MasterChef is a decently well trusted contract and all JLP rewards are distributed there. Fundamentally the number should not be off by any, if any will be dust, and this exists to protect in the worst case so at least some users can get JOE out. However it is a backstop and extra safety measure. In #137 the reward being off by 10 would require an additional bug somewhere else, or a failure of MasterChef.
#3 - alcueca
2022-01-15T06:38:13Z
Duplicate of #137
🌟 Selected for report: jayjonah8
531.2947 USDC - $531.29
jayjonah8
the constructor in TellorCaller.sol should ensure that the _tellorMasterAddress arg passed in is not a zero address as a safegaurd.
Manual code review
require(address(_tellorMasterAddress) != address(0), "Zero address")
🌟 Selected for report: jayjonah8
97.5905 USDC - $97.59
jayjonah8
GasPool.sol is not used and should be removed to reduce unnecessary files.
Manual code review
#0 - kingyetifinance
2022-01-04T15:54:38Z
@LilYeti: We are likely still using this for the purpose stated in the file.
#1 - alcueca
2022-01-16T21:02:07Z
@kingyetifinance, I don't understand why you need to deploy an empty contract to hold assets, instead of just having an EOA. Is it to ensure that no one ever has the private key for it?
In any case, sponsor acknowledged the issue.
17.7859 USDC - $17.79
jayjonah8
The WJLP.sol file imports SafeMath.sol to use in the file which is not necessary because solidity automatically includes SafeMath by default in versions 0.8.0 and above. The WJLP.sol file uses solidity 0.8.7 so the extra code and import is not needed.
https://soliditydeveloper.com/solidity-0.8
Manual code review
Remove the SafeMath.sol import in WJLP.sol and the SafeMath.sol file can completely be removed if all files in the project use solidity 0.8.0 or above. This can save a lot on gas
#0 - alcueca
2022-01-15T06:55:12Z
It doesn't actually save a lot of gas. It saves a bit of gas. The main benefit from checked arithmetic in solc 0.8+ is on code clarity and safety from checked arithmetic by default, unchecked as optional. Taking #246 as main.
🌟 Selected for report: jayjonah8
97.5905 USDC - $97.59
jayjonah8
The BaseBoringBatchable.sol file seems to not be used anywhere and if this is the case it should be removed to save gas and also because it's been the source of some horrible bugs due to protocols using it with msg.value creating double spending bugs.
https://samczsun.com/two-rights-might-make-a-wrong/
Manual code review
Remove BaseBoringBatchable.sol if it's not being used.
#0 - 0xtruco
2022-01-14T07:44:43Z
We just won't deploy it
🌟 Selected for report: defsec
Also found by: 0x1f8b, Jujic, WatchPug, broccolirob, certora, cmichel, csanuragjain, hyh, jayjonah8, kenzo, robee, sirhashalot
jayjonah8
Some tokens don't correctly implement the EIP20 standard and their transfer/transferFrom function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Making use of safeTransfer/safeTransferFrom act as a safe guard and can prevent future bugs when protocols are updated as well.
Manual code review
Its recommend to use OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.
#0 - kingyetifinance
2022-01-03T06:29:11Z
Duplicate issue #1
#1 - alcueca
2022-01-15T07:33:15Z
Duplicate of #94