Platform: Code4rena
Start Date: 09/12/2021
Pot Size: $50,000 USDC
Total HM: 19
Participants: 21
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 61
League: ETH
Rank: 13/21
Findings: 3
Award: $1,014.14
🌟 Selected for report: 5
🚀 Solo Findings: 0
🌟 Selected for report: sirhashalot
Also found by: Jujic, sirhashalot
380.2618 USDC - $380.26
sirhashalot
There are instances where the SafeApprove() function is called only once without setting the allowance to zero. Some tokens, like USDT, require first reducing the address' allowance to zero by calling approve(_spender, 0). Additionally, the comment before the OpenZeppelin SafeApprove function states "safeApprove should only be called when setting an initial allowance, or when resetting it to zero".
Transactions will revert when using an unsupported token like USDT (see the approve() function requirement at line 199).
Note: the usage of SafeApprove() in mocks/yVault/Controller.sol (lines 196-197) does not need modification since it already uses the recommended approach.
Manual analysis
Use SafeApprove() to set the allowance to zero first. Alternatively, use safeIncreaseAllowance or safeDecreaseAllowance instead as suggested by the OpenZeppelin SafeApprove comment.
#0 - 0xean
2022-01-21T21:18:13Z
dupe of #97
🌟 Selected for report: sirhashalot
Also found by: Jujic, sirhashalot
380.2618 USDC - $380.26
sirhashalot
There are 3 instances where the IERC20.approve() function is called only once without setting the allowance to zero. Some tokens, like USDT, require first reducing the address' allowance to zero by calling approve(_spender, 0). Transactions will revert when using an unsupported token like USDT (see the approve() function requirement at line 199).
Note: the usage of approve() in yield/CompoundYield.sol (lines 211-212), in yield/YearnYield.sol (lines 211-212), and in yield/AaveYield.sol (lines 297-298) do not need modification since it they already use the recommended approach. Additionally the usage of approve() in yield/AaveYield.sol:307 likely does not need modification since that approve function only handles ETH.
Manual analysis
Use approve(_spender, 0) to set the allowance to zero immediately before each of the existing approve() calls.
#0 - 0xean
2022-01-21T21:17:25Z
moving to medium risk as the availability of the protocol is affected.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
🌟 Selected for report: sirhashalot
51.3353 USDC - $51.34
sirhashalot
The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.
Detailed description of the impact of this finding.
Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219.
The deprecated function is found in:
Manual analysis
As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.
🌟 Selected for report: sirhashalot
281.6754 USDC - $281.68
sirhashalot
The closePoolExtension() function in contracts/Pool/Extension.sol deletes an ExtensionVariables struct, but the mapping in the ExtensionVariables struct is not deleted. Values from this “deleted” mapping can still be accessed and there are no comments, function names, or other indicators that this is known. This issue is documented in Solidity’s security considerations documentation:
https://docs.soliditylang.org/en/v0.8.10/security-considerations.html#clearing-mappings
The delete operation occurs in contracts/Pool/Extension.sol at line 175 https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Extension.sol#L175 The mapping that still exists after the delete is the lastVotedExtension mapping at line 24: https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Extension.sol#L24
Slither mapping-deletion detector https://github.com/crytic/slither/wiki/Detector-Documentation#deletion-on-mapping-containing-a-structure
The Slither documentation suggests using a lock mechanism instead of a deletion. Alternatively, the Solidity official documentation suggests using something similar to this iterable mapping contract: https://github.com/ethereum/dapp-bin/blob/master/library/iterable_mapping.sol
#0 - ritik99
2021-12-23T10:08:34Z
We're aware that the mapping doesn't get deleted and have correspondingly taken appropriate measures while using lastVotedExtension throughout the contract
🌟 Selected for report: sirhashalot
Also found by: 0x0x0x, WatchPug, cmichel, pmerkleplant, robee
8.2172 USDC - $8.22
sirhashalot
uint256 variable are initialized to a default value of zero per Solidity docs: https://docs.soliditylang.org/en/latest/control-structures.html#default-value
Setting a variable to the default value is unnecessary. Removing lines of code where variables are initialized to zero can save gas. Here are a few articles describing this gas optimization: https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#53bd https://medium.com/coinmonks/gas-optimization-in-solidity-part-i-variables-9d5775e43dde#4135
Manual analysis
Instead of initializing a variable to zero, such as uint256 abc = 0;, the line can be shortened to uint256 abc; as Solidity automatically initializes uint variables to zero.
🌟 Selected for report: 0xngndev
Also found by: Jujic, WatchPug, robee, sirhashalot
10.9563 USDC - $10.96
sirhashalot
Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas (as documented publicly)
There are dozens of examples of this gas optimization opportunity in the project, but some examples of this issue include:
A more complete list of all instances of this issue can be provided upon request.
Manual analysis
Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met.
#0 - ritik99
2021-12-25T16:42:30Z
Duplicate of #47
🌟 Selected for report: sirhashalot
281.6754 USDC - $281.68
sirhashalot
The public calculateInterest function in CreditLine.sol is missing a @param comment for the _timeElapsed parameter. This parameter is obviously important and the units should be clearly stated as seconds.
The calculateInterest function in CreditLine.sol:
https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/CreditLine/CreditLine.sol#L385-L395
Manual analysis
Add the following line to the calculateInterest function comments in CreditLine.sol:
* @param _timeElapsed Seconds elapsed since lastPrincipalUpdateTime