Open Dollar - 0xprinc's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODProxy.sol#L26-L35 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L49-L53

Vulnerability details

Impact

ODProxy which is also the _safeOwner will not be able to call any relevant function in ODSafeManager.sol.

Proof of Concept

The _safeOwner in ODSafeManager.sol refers to the proxy contract address related to the original user. This is confirmed in the function openSAFE() where vault721.mint(_usr, _safeId) is called. Here _usr is the address of the ODProxy. Now the issue is that ODProxy.sol, which is the proxy contract of the original holder of NFT/safe. It doesn't have a function that does normal low-level call and instead have an Execute() function which only does a delegatecall which forces this contract to never become msg.sender of any call it does.

Now consider the initial stages of the contract when a new NFT/safe has been minted and is mapped under an ODProxy/_safeOwner. The ODProxy/_safeOwner will not be able to call any function that has the _safeAllowed() modifier. Because _safeOwner is a contract and will only do a delegate call and hence can not call the function allowSAFE()

msg.sender == _safeData[_safe].owner 

will never be satisfied because the msg.sender will never be _safeOwner but the original owner of the Proxy and NFT/Safe.

This will lead to the _safeOwner stuck in the contract and not able to call any of these functions even if these are meant to be called by it

allowSAFE() -> this will not even let proxy to let any other use those functions modifySAFECollateralization() transferCollateral() transferCollateral() transferInternalCoins() quitSystem() enterSystem() moveSAFE() addSAFE() removeSAFE() protectSAFE()

which is almost all functions that are relevant to the SAFE.

Tools Used

Manual Review

Recommended Mitigation Steps

Some mitigations can be

1. Consider _safeOwner to be the real owner of the NFT which is also the ODProxy.OWNER. Change the mapping to contain this real owner as this will resolve this issue.
2. Add another function in the ODProxy.sol which include a normal low-level call. This will introduce another issue that since the proxy will become msg.sender and then it will have another proxy of itself while minting or transfering NFT/safe, since the protocol will treat it as another address without a proxy attached to it. This can be mitigated introducing a mapping in Vault721.sol

mapping (address Proxy => bool) isProxy

This will be true for the addresses that are proxies and will be made true when the proxy is deployed for a contract and will check that if a contract is proxy then another proxy is not deployed for it.

Assessed type

DoS

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L94-L99

Vulnerability details

Impact

Minting of the New Safes/NFTs from Vault721.sol will revert, leading to a temporary DOS.

Proof of Concept

The function Vault721.sol is used to mint the new safes. This function is only called by the safeManager. The current implementation of the ODSafeManager.sol is such that variable _safeId keeps check of the number of Safes that have been deployed. And this variable starts from 0 whenever a new ODSafeManager is deployed. So when the governor of Vault721.sol update the address of SafeManager, and a new ODSafeManager.sol contract is assigned to it, _safeId will initialize from 0 (as the ODSafeManager.sol have a fixed address of Vault721). So, when a new safe is minted its tokenId/safeId will be 0, which will revert the _safeMint function that is present in Vault721.sol/mint(), because the tokenId is already minted earlier.

This can be escalated when a malicious user front-runs the Vault721.sol/initiaizeManager() function at the early stages of the contract and non-zero safes/NFTs, making a DOS situation when the governor changes the manager contract manually to ODManager.sol, because the mint will now always revert with this implementation of ODSafeManager.sol.

Tools Used

Manual Review

Recommended Mitigation Steps

There should present a function to directly change the value of ODSafeManager.sol/_safeId state variable or the function to sync the tokens minted and _safeId. Access given to Vault721.sol.

Function present in ODSafeManager.sol

    function sync(uint _newSafeId) external {
        require(msg.sender == address(vault721), 'SafeMngr: Only Vault721');
        _safeId = _newSafeId;
    }

Function present in Vault721.sol,

       
    uint internal totalMinted;

    function syncSend() external onlyGovernor{
        IODSafeManager(safeManager).sync(totalMinted);
    }

and should also include the code inside the mint function to update the totalMinted.

Assessed type

DoS

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/SAFEHandler.sol#L11 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L59-L62 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L112-L115 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L118 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L189 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L205

Vulnerability details

Impact

All the functions with handlerAllowed modifier are unusable and can never be called.

Proof of Concept

Handler is deployed and assigned to the Safe/NFT when ODSafeManager.sol/openSAFE() function is called. The SafeHandler.sol have only a constructor and not any function so it can not do any external call and hence can not become a msg.sender for any transaction.

Now, the function ODSafeManager.sol/allowHandler() can not be called by the SafeHandler.sol because it don't have a function to do external call. This means it can not approve any other address also.

mapping(address _safeHandler => mapping(address _caller => uint256 _ok)) public handlerCan;

function allowHandler(address _usr, uint256 _ok) external {
    handlerCan[msg.sender][_usr] = _ok;

So, the modifier handlerAllowed() will always fail as there is no one allowed by safeHandler and safeHandler can not do an external call by itself.

  modifier handlerAllowed(address _handler) {
    if (msg.sender != _handler && handlerCan[_handler][msg.sender] == 0) revert HandlerNotAllowed();
    _;
  }

And hence every function containing this modifier will not execute making them non-functional.

Functions include: quitSystem() enterSystem()

Tools Used

Manual Review

Recommended Mitigation Steps

1. The safeHandler should contain an OWNER variable which is changed after the NFT/safe is transferred to some other user. There should also be present a function(only be called by the OWNER) to do an external call to other contracts so that the following issue can be mitigated.

Assessed type

DoS