Open Dollar - Baki's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L199

Vulnerability details

Impact

The protocol has a feature to auction the surplus amount and send some % out of that to extraSurplusReceiver but currently this % always less than 1% which limits the amount extraSurplusReceiver can get out of the surplus

Proof of Concept

we believe this check is incorrect if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit(); since this basically means that surplusTransferPercentage cannot be more than 1% thereby adding an unnecessary restriction in the protocol

Tools Used

manual review

Recommended Mitigation Steps

change this check if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit(); to if(_params.surplusTransferPercentage > ONE_HUNDRED_WAD) revert AccEng_surplusTransferPercentOverLimit();

Assessed type

Invalid Validation

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L112 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/SAFEHandler.sol#L11

Vulnerability details

Impact

The ODSafeManager has a mapping handlerCan where according to the docs the key is the SAFEHandler contract address but first of all, there is no access control check that msg.sender in allowHandler is an existing SAFEHandler contract and secondly SAFEHandler has no method to call allowHandler method in safe manager which means anyone can update the mapping and handlerCan is used in a modifier handlerAllowed which is used for access control in various methods involving safe related operations like quitSystem & enterSystem and hence this breaks the access control check in these functions

Proof of Concept

there is no method to call allowHandler in SAFEHandler and secondly alice can call allowHandler and pass any arbitary address there even her own address as the _usr argument and break the handler access control

Tools Used

manaual review

Recommended Mitigation Steps

add a method allowHandler in SAFEHandler that can only be called by the owner of the safe and also add access control check in allowHandler that msg.sender is a valid safe by passing an extra safeOwner address in the method to verify that

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

There is no access control while initialising safe manager & nftRenderer so mev bots can monitor the deployment of Vault721 contract and initialise both safe manager & nftRenderer with malicious addresses and brick the protocol until the governer can update the addresses

Proof of Concept

The mev bot can have the following contracts set as safe manager and NFT renderer

contract MaliciousManager {}

contract MaliciosNFTRenderer {}

now this would mean that no one would be able to mint a safe since there is no transferSAFEOwnership method until the governer updates the addresses

Tools Used

manual review

Recommended Mitigation Steps

add access control in the initialize methods and hence add onlyGovernor modifier there too so avoid any sort of hindrance to the protocol

Assessed type

Access Control