Variable rate AuditFixes #547
Conversation
| @@ -3,4 +3,4 @@ | |||
| forge-std/=lib/forge-std/ | |||
| erc3156/=lib/ERC3156/ | |||
| dss-interfaces/=lib/dss-interfaces/ | |||
| openzeppelin/=lib/openzeppelin-contracts/ | |||
| @openzeppelin/=lib/openzeppelin-contracts/ | |||
There was a problem hiding this comment.
Why do we need node convention here?
| } | ||
|
|
||
| // Manage base | ||
| if (base != 0) { | ||
| IJoin baseJoin = getJoin(vault.baseId); | ||
| if (base < 0) baseJoin.join(vault.owner, uint128(-base)); | ||
| if (base > 0) baseJoin.exit(to, uint128(base)); | ||
| else baseJoin.exit(to, uint128(base)); |
There was a problem hiding this comment.
What if base is 0? That wasn't being checked before but is now?
There was a problem hiding this comment.
On 325 we are checking that base != 0
| @@ -297,26 +297,6 @@ contract VRLadle is UUPSUpgradeable, AccessControl() { | |||
|
|
|||
| // ---- Asset and debt management ---- | |||
|
|
|||
| /// @dev Move collateral and debt between vaults. | |||
| function stir( | |||
There was a problem hiding this comment.
Why was stir moved exactly?
There was a problem hiding this comment.
It doesn't update the global debt counters when ilkId != baseId. It could be fixed, but at the same time we haven't ever used it, so better to just reduce the attack surface.
| @@ -259,75 +255,3 @@ contract VYTokenTest is VYTokenZeroState { | |||
| assertLe(principalAmount, vyToken.convertToPrincipal(INK)); | |||
| } | |||
| } | |||
|
|
|||
| contract FlashLoanEnabledStateTests is FlashLoanEnabledState { | |||
There was a problem hiding this comment.
Why remove all these tests exactly?
There was a problem hiding this comment.
Since, flash loan was removed the tests are removed as well.
| @@ -154,18 +154,6 @@ contract VaultTests is VaultBuiltState { | |||
| ladle.give(vaultId, admin); | |||
| } | |||
|
|
|||
| function testOnlyOwnerCouldMove() public { | |||
There was a problem hiding this comment.
Why remove so many tests from this file? Is it because they're already tested elsewhere?
There was a problem hiding this comment.
The tests related to stir were removed.
This PR contains the fixes based on the reports the auditors submitted. The fixes were first implemented in the auditor-specific repos and then after their approval were aggregated and implemented in this repo. You can find the reports and PRs here:
We need to review if all the accepted changes in the above reports were implemented in this one.