Open Dollar - 0xPsuedoPandit's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L136-L152

Vulnerability details

Impact

The users who should not allowed to have access of the safe can access it and make possible changes to the safe which might result in fund loss for the owner in an edge case.

Proof of Concept

In transferSafeOwnership function of ODSafeManager::transferSAFEOwnership, makes the transfer of safe ownership and updates the owner, now ideally the users allowed by the owner should not have access to the safe and associated functions after transferring ownership, but there is an edge case, let's break it down step by step:

1. Open a safe and become the owner by openSafe function, now your proxy address has the access to the safe.

2. By allowSafe function, let's say you have allowed multiple users to access your safe on your behalf. https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105-L109 safeCan[_owner][_safe][_usr] = _ok;

3. Now transferSafeOwnership function leaves behind an inconsistent state, it indeed updates the owner but leaves behind inconsistent states, it does not clears the access of previous owner has given to the users, it only changes the current owner. https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L149

4. At some point in the future this initial owner again gains ownership of this safe then all the previous approvals will be considered valid because it was not cleared while transferring ownership.

lets take a example to better understand this issue: we have a mapping, mapping(address a => mapping(address b => unit c)) private safeCan; [a][b] = 1; // a being the owner gives access to his safe to user b. [c][b] = 0; // since is replaced B's access will be taken away. [a][b] = 1; // because safeCan and handlerCan have got inconsistent states, there previous approvals will still work against the will of the owner this time.

This breaks the invariant mentioned "If the ERC-721 token from Vault721 is transferred, so too is the ownership and control of the corresponding safe."

If the user can bypass safeAllowed and hadlerAllowed modifiers he has access to the very vital functions of the safe for example: modifySAFECollateralization transferCollateral transferInternalCoins quitSystem enterSystem and more..

I would like to mention again this time it is going to be against the will of the owner who previously gave access to them but this fact of access to the safe is not known to him, by modifying collateral someone can put the safe at the verge of liquidation and anything could be possible with having access to malicious user.

The likelihood of this edge case is not much but it is still possible and severity is undoubtedly high, inconsistent state should never be left behind anyways.

Tools Used

Manual

Recommended Mitigation Steps

Clear all the previous access to the safe after transferring it, no inconsistent states should be left behind.

Assessed type

Access Control

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L118-L133

Vulnerability details

Impact

If the collateral provided by the user, is a token with low value and liquidity, then it might affect protocol and the user itself in different ways including making the safe undercollateralized.

Proof of Concept

The openSafe function of ODSafeManager takes in two parameters _cType and _usr, it is wrongly assumed by the protocol that the collateral provided by the user is indeed the collateral he is supposed to provide. There is no whitelisting of collateral tokens but it is mentioned that the collateral should be liquid staking tokens and moreover, they will deploy with the following tokens ARB, WSTETH, CBETH, RETH, and MAGIC after that, new collaterals need to be voted in by the DAO, it can further be queried by calling in CollateralJoinFactory::collateralTypesList() also, in CollateralAuctionHouseFactory::collateralList()

The openSAFE function in the ODSafeManager does not directly handle the collateral. Instead, it creates a new SAFE (Single Collateral Debt Position) and assigns it to a user. The type of collateral that can be used is determined by the _cType parameter, which is a bytes32 value representing the collateral type. But nothing is stopping the user from providing collateral of low value and low liquidity which can lead to several potential issues.

1. If collateral loses value rapidly, SAFE could become undercollateralized. This means the value of the debt in the SAFE is greater than the value of the collateral. In this case, SAFE could be liquidated.

2. If the coin has low liquidity, it might be difficult to sell the collateral in the event of a liquidation, which could potentially cause issues in the system.

3. It makes the whole purpose of the protocol absurd as it intends to provide debt against liquid staking tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Use whitelisting of collateral tokens that are allowed in the system as well as allowed for taking debt, and if the collateral is not from the list revert the transaction.

Assessed type

Invalid Validation