Platform: Code4rena
Start Date: 16/11/2021
Pot Size: $30,000 USDC
Total HM: 3
Participants: 18
Period: 3 days
Judge: leastwood
Total Solo HM: 2
Id: 56
League: ETH
Rank: 5/18
Findings: 3
Award: $1,906.60
🌟 Selected for report: 5
🚀 Solo Findings: 0
1014.5458 USDC - $1,014.55
TimmyToes
Potential increased financial loss during security incident.
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/Alchemist.sol#L611 Consider a critical incident where a vault is being drained or in danger of being drained due to a vulnerability within the vault or its strategies. At this stage, you want to trigger emergency exit and users want to withdraw their funds and repay/liquidate to enable the withdrawal of funds. However, minting against debt does not seem like a desirable behaviour at this time. It only seems to enable unaware users to get themselves into trouble by locking up their funds, or allow an attacker to do more damage.
Convert emergency exit check to a modifier, award wardens who made that suggestion, and then apply that modifier here.
Alternatively, it is possible that the team might want to allow minting against credit: users minting against credit would effectively be cashing out their rewards. This might be seen as desirable during emergency exit, or it might be seen as a potential extra source of risk. If this is desired, then the emergency exit check could be placed at line 624 with a modified message, instructing users to only use credit.
57.0552 USDC - $57.06
TimmyToes
The event emitted is for the updating of a different fee (the harvest fee). This could cause potential issues for any system wishing to integrate with yAxis and wishing to monitor changes to the system and potentially react to them. Such a system could record the wrong harvest fee and would be unaware of updates to the borrow fee.
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/Alchemist.sol#L299 Is the same as line 284
Create a new event: event BorrowFeeUpdated(uint256 borrowfee); and call it on line 299 instead of HarvestFeeUpdated
21.2967 USDC - $21.30
TimmyToes
Including unused libraries could potentially use up gas and certainly makes the code more difficult to understand, hindering developer integrations/poor confused security auditors.
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L19 https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/adapters/YearnVaultAdapter.sol#L19 The contract does not use FixedPointMath and compiles with these lines removed.
Remove line 10,19 from each contract (FixedPointMath lines). I'd also prefer the removal of import "hardhat/console.sol"; but this is not having any impact and is just to tidy and shorten the files.
🌟 Selected for report: TimmyToes
47.3259 USDC - $47.33
TimmyToes
Increased gas usage
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/Alchemist.sol#L630 _cdp.totalDebt is a storage variable that is being assigned to twice in this block.
To save gas, use a local memory variable to calculate the new totalDebt and then make a single assignment to _cdp.totalDebt from that memory variable.
#0 - Xuefeng-Zhu
2021-11-23T08:14:50Z
use local variable will increase complexity.
#1 - 0xleastwood
2021-12-21T06:01:51Z
Agree with sponsor but the finding is valid nonetheless, hence I will leave it open.
🌟 Selected for report: TimmyToes
751.5154 USDC - $751.52
TimmyToes
Lots of unnecessary wasted gas on failing transactions. Potential source of system manipulation if xtoken is malicious as it is called with a partially updated state at line 631.
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/Alchemist.sol#L636 Best practice is to generally follow the check-effect-interactions pattern. Line 636 is the main check for whether the transaction is possible and it follows after both effects (state updates) and external interactions (xtoken.mint).
I have no specific exploit for a malicous xtoken because this competition is of limited scope. It is mitigated by the nonReentrant modifier and the possibility that all xtokens will be trustworthy. However, it is conceivable that a malicious xtoken could make calls elsewhere in the system that might pose a security risk.
Ideally, rewrite the check on line 636 to be possible without storage writes and perform this check before updating state variables or calling external functions. Mint to rewards at line 640.
If the team is happy with the high extra costs of failed transactions and does not wish to rewrite the check on line 636 to be possible without storage writes first, then I would just make one recommendation:
Make extra sure that the interface to this contract correctly calculates the permitted max value for _amount. Standard users will have a very negative experience if the javascript interface gives them the wrong number.
As a halfway measure, there would be an easily achievable high gas saving for reverting transactions if the tokens minted on 631 were instead minted after the _cdp.checkHealth() call, (although this would slightly increase gas for transactions that pass that check).
TimmyToes
Save gas on all code that uses vault.
https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L24 https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/adapters/YearnVaultAdapter.sol#L24 This variable is only set in the constructor. It is a perfect candidate for immutable.
The constructor would have to be rewritten to include the code of updateApproval():
constructor(IVault _vault, address _admin) public { vault = _vault; admin = _admin; address _token = _vault.getToken(); IDetailedERC20(_token).safeApprove(address(_vault), uint256(-1)); }
(in this case, the original updateApproval() should not be removed as its functionality would still be needed.) This would increase deployment costs, but vault is used many times throughout the contract. It would be a good gas saving.
#0 - Xuefeng-Zhu
2021-12-09T06:48:00Z
TimmyToes
_expectCaller() can be restricted to view to enable possible gas optimisations.
#0 - Xuefeng-Zhu
2021-11-23T08:11:39Z
_expectCaller is not used and removed
#1 - Xuefeng-Zhu
2021-12-09T06:53:30Z