Open Dollar - nmirchev8's results
A floating $1.00 pegged stablecoin backed by Liquid Staking Tokens with NFT controlled vaults.
General Information
Platform: Code4rena
Start Date: 18/10/2023
Pot Size: $36,500 USDC
Total HM: 17
Participants: 77
Period: 7 days
Judge: MiloTruck
Total Solo HM: 5
Id: 297
League: ETH
Findings Distribution
Researcher Performance
Rank: 5/77
Findings: 3
Award: $2,356.66
🌟 Selected for report: 1
🚀 Solo Findings: 1
Findings Information
Awards
21.9995 USDC - $22.00
Labels
External Links
Lines of code
Vulnerability details
Summary
Each user has a proxy, which is the owner of the user corresponding safes. The proxy is used so many transactions could be combined together and so safe gas. The problem here is that if there is no "delegator" between the user's proxy and the ODSafeManager all call will revert, because safeAllowed(_safe) modifier compares msg.sender with the _safe's owner, who is the proxy address of the original user address. Note that if the user executes calls via his proxy directly to the ODSafeManager msg.sender would be the actual user(because of the delegatecall) and not the proxy contract. Furthermore - all changes from this call won't affect the storage of the manager contract.
Impact
The following functions are not executable in the current protocol context:
- allowSAFE()
- quitSystem()
- enterSystem()
- moveSAFE()
- removeSAFE()
- protectSAFE()
Proof of Concept
Here is a test presenting the impossibility to allow another address to manage a safe:
Run it inside test/nft/anvil/NFT.t.sol using forge t --fork-url $ANVIL_RPC --match-contract NFTAnvil --match-test test_setSafeManagerWillDoSFunctionalities -vvvv
Import import {ODProxy} from '@contracts/proxies/ODProxy.sol'; if necessary
Tools Used
Manual Review
Recommended Mitigation Steps
Add missing functions to the "delegator" BasicActions.sol
Assessed type
call/delegatecall
#0 - c4-pre-sort
2023-10-26T03:54:37Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-10-26T03:54:49Z
raymondfam marked the issue as duplicate of #76
#2 - c4-pre-sort
2023-10-26T19:04:19Z
raymondfam marked the issue as duplicate of #380
#3 - c4-judge
2023-11-02T18:16:57Z
MiloTruck marked the issue as not a duplicate
#4 - c4-judge
2023-11-02T18:17:09Z
MiloTruck marked the issue as duplicate of #294
#5 - c4-judge
2023-11-08T00:27:03Z
MiloTruck marked the issue as satisfactory
Findings Information
Labels
Awards
112.1671 USDC - $112.17
External Links
Lines of code
Vulnerability details
Summary
In Vault721 it is possible to change the implementation of SafeManager, which tightly coupled to the NFV. The function is only callable by the governor, but when it is called it could lead to blocked functionality to open new safes(Mint), fund loses, or other unexpected behaviours.
Impact
I marked the following finding high as there are multiple scenarios with this root, which could lead to DoS, lost of funds and wrong accounting calculations.
- DoS - of the
openSafefunctionality - Lost of funds/Wrong accounting - if transfer(id) is called on the new implementation, safe information would be changed to new owner and then implementation is switched back to the old version, to resume the possibility to mint tokens (the ownership of the safes for all transfers that had happend on different implementation, won't match the actual ownership of the nfts, which is a funds loss for one of the parties in the trade that happened)
- Unexpected behaviour - Even if all above cases are prevented, still a problem is that the old safeManager is active and it could be directly accessed to open new safes, or call SafeEngine accounting functions.
Proof of Concept
Execute PoC:
Inside test/nft/anvil/NFT.t.sol import import {ODSafeManager} from '@contracts/proxies/ODSafeManager.sol'; import {ODProxy} from '@contracts/proxies/ODProxy.sol'; at the top and then place the following test
forge t --fork-url $ANVIL_RPC --match-contract NFTAnvil --match-test test_setSafeManagerWillDoSFunctionalities -vvvv
Tools Used
Manual Review
Recommended Mitigation Steps
I don't know what is actually the best solution here. Maybe an architecture design change, where safe data is always stored and a proxy implementation, which would be the ODSafeManager address to be change inside Vault721
Assessed type
ERC721
#0 - c4-pre-sort
2023-10-26T01:39:15Z
raymondfam marked the issue as low quality report
#1 - c4-pre-sort
2023-10-26T01:39:30Z
raymondfam marked the issue as duplicate of #37
#2 - c4-judge
2023-11-01T20:41:23Z
MiloTruck marked the issue as not a duplicate
#3 - c4-judge
2023-11-01T20:41:32Z
MiloTruck marked the issue as duplicate of #233
#4 - c4-judge
2023-11-08T00:03:29Z
MiloTruck marked the issue as not a duplicate
#5 - c4-judge
2023-11-08T00:03:37Z
MiloTruck marked the issue as unsatisfactory: Invalid
#6 - c4-judge
2023-11-08T00:03:42Z
MiloTruck marked the issue as unsatisfactory: Insufficient proof
#7 - MiloTruck
2023-11-08T00:04:00Z
This report is overly generic and doesn't pinpoint exactly what the bug is.
#8 - c4-judge
2023-11-08T00:07:37Z
MiloTruck changed the severity to 2 (Med Risk)
#9 - NicolaMirchev
2023-11-10T17:50:09Z
Hey, @MiloTruck Why the report is marked as unsatisfactory, when the explanation and impact are clear, there is a coded, working PoC and dup of this are valid, without something more specific. "when it is called it could lead to blocked functionality to open new safes(Mint), fund loses" Here is a pointing of what the bug is - calling the function to change manager. Also I have provided different impacts with examples:
Lost of funds/Wrong accounting - if transfer(id) is called on the new implementation, safe information would be changed to new owner and then implementation is switched back to the old version, to resume the possibility to mint tokens (the ownership of the safes for all transfers that had happend on different implementation, won't match the actual ownership of the nfts, which is a funds loss for one of the parties in the trade that happened)
This is not generic. It is detailed explanation of possible scenario with bad impact. Please review the explanations and examples given in the report. Thank you.
#10 - c4-judge
2023-11-11T08:28:06Z
MiloTruck removed the grade
#11 - c4-judge
2023-11-11T08:28:44Z
MiloTruck marked the issue as duplicate of #381
#12 - c4-judge
2023-11-11T08:28:58Z
MiloTruck marked the issue as partial-50
#13 - MiloTruck
2023-11-11T08:34:50Z
when the explanation and impact are clear
Your report isn't as clear as you think it is, without looking at the coded PoC it is hard to tell which specific part of the codebase you're referring to or what the bug is.
All other duplicates that received full credit had either:
- Included code snippets to explain where the bug is, and how it could occur.
- Contained a step-by-step explanation of how the bug could occur.
In contrast, your report contains none of the above.
I will consider it valid with partial credit in favor of the coded PoC, but please do write your issues in more detail for future submissions.
#14 - NicolaMirchev
2023-11-11T21:09:40Z
Hey, @MiloTruck Thank you for your consideration and explanation, but I still think that the points you have mention are fulfilled in my report. Explanation and step by step for the bug are both implemented inside the test case. A coded PoC, which end state is the impact of the vulnerability contains detailed step by step (100% accurate example) explanation and points out exactly the code functions and their order of execution to reproduce such. Also I have provided additional comments in the test and a screenshot of the function execution stack of the vulnerable impact.
#15 - MiloTruck
2023-11-11T23:19:31Z
@NicolaMirchev I have to disagree here.
Providing a coded PoC with two comments and a screenshot that it reverts does NOT make for a sufficient explanation.
It only proves that there is a bug somewhere, but doesn't highlight what the bug is or which part of the code is incorrect.
My decision here is final.
Findings Information
🌟 Selected for report: nmirchev8
Labels
Awards
2222.4853 USDC - $2,222.49
External Links
Lines of code
Vulnerability details
Summary
In the current implementation each safe has corresponding proxy contract inside ODSafeManager and corresponding SAFEHandler "proxy" which is deployed for each safe and used as safe's address inside SAFEEngine.
The problem is that inside ODSafeManager transferCollateral() function there is no check wether the provided address is a valid safe and so the collateral could be "transferred" to any ordinary address.
Impact
This could result in contracts missbehaviour, or lost of funds as it breaks the invariant of "Each safe has a corresponding SAFEHandler" and so other accounting errors could appear as the new owner of the contract can transfer it where he wants directly calling SafeEngine.sol contract, which is also unwanted behaviour (To execute safe operations, bypassing SafeManager.sol)
Proof of Concept
- Alice has a safe with X amount of collateral and want to transfer it to Bob and passes his address.
- The transaction passes and now inside SAFEEngine Bob's amount of collateral has increased, but if he want to do something with safe/s he has to have a proxy and a safe, for which he would be owner. So those "funds" which are for accounting don't have corresponding vault and safe. They are assigned to an EOA
Tools Used
Manual Review
Recommended Mitigation Steps
The issue is confirmed by sponsor, who came up with a prevention:
Assessed type
Invalid Validation
#0 - c4-pre-sort
2023-10-26T06:40:08Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-10-26T06:40:12Z
raymondfam marked the issue as primary issue
#2 - c4-pre-sort
2023-10-27T05:57:48Z
raymondfam marked the issue as high quality report
#3 - c4-sponsor
2023-10-31T17:49:20Z
pi0neerpat (sponsor) confirmed
#4 - c4-judge
2023-11-02T17:38:22Z
MiloTruck marked the issue as satisfactory
#5 - c4-judge
2023-11-02T17:38:26Z
MiloTruck marked the issue as selected for report
#6 - MiloTruck
2023-11-02T17:41:20Z
The warden has demonstrated how due to a lack of validation on the _dst parameter in transferCollateral() and transferInternalCoins(), the internal accounting of collateral and coins in the SAFEEngine contract can accrue to any arbitrary EOA, such as an attacker's own address. This can be exploited to call functions in the SAFEEngine contract directly, which should not be possible.
Since this report does not demonstrate how this could lead to a potential loss of funds for the protocol/users, I believe medium severity is appropriate.
#7 - DevHals
2023-11-10T20:39:54Z
Hi,
I think this is intended by design to transfer SAFE collateral to any address that doesn't represent a SAFE (safeHandler), as per the sponsors reply below:
#8 - MiloTruck
2023-11-11T08:10:02Z
@pi0neerpat could you confirm if users should be able to call transferCollateral() and transferInternalCoins() in ODSafeManager to transfer their internal coin/collateral balance in SAFEEngine to any arbitrary address?
The internal coins/collateral will not be stuck in the protocol as users can call exit() in CollateralJoin or CoinJoin to mint system coins or transfer out collateral.
However, allowing internal balances to accrue to any arbitrary address means that users can call functions in the SAFEEngine contract directly, instead of through ODSafeManager. This was my original reason for medium severity, so it would be great if you could clarify if this should not be allowed.
#9 - NicolaMirchev
2023-11-11T08:30:01Z
In such way users can call functions on SAFEEngine contract directly, which breaks the invariants provided by the team.
#10 - daopunk
2023-11-14T01:19:10Z
I agree that user calls directly to the SafeEngine should not be allowed, thus a check that the destination address is an existing safeHandler should be made.
#11 - MiloTruck
2023-11-14T03:58:27Z
Will maintain medium severity since this is unintended behavior, as mentioned by the sponsor, and seems to violate the following invariant from the contest's README:
Users must exclusively use the ODProxy to interact with their safes.