Open Dollar - MrPotatoMagic's results

Lines of code

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

Vulnerability details

Impact

The function allowSafe in the ODSafeManager.sol contract is intended to be called directly from a user's ODProxy address. But since the execute() function calls the allowSafe() function using delegateCall (which does not change the msg.sender context to ODProxy instead of user's address), the user is not able to give safe permissions to someone since the safeAllowed() modifier reverts. Furthermore, due to this issue, all functions in the ODSafeManager.sol contract that depend on the safeAllowed() modifier revert.

Proof of Concept

Here is the whole process:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODProxy.sol#L26

1. User (EOA) calls execute() function in their ODProxy with address _target parameter as ODSafeManager contract and bytes memory data parameter as the function signature for allowSafe() function (along with values).

File: ODProxy.sol
26:   function execute(address _target, bytes memory _data) external payable onlyOwner returns (bytes memory _response) {
27:     if (_target == address(0)) revert TargetAddressRequired();
28: 
29: 
30:     bool _succeeded;
31:     (_succeeded, _response) = _target.delegatecall(_data);
32: 
33: 
34:     if (!_succeeded) {
35:       revert TargetCallFailed(_response);
36:     }
37:   }

2. The ODProxy of the user delegate calls the allowSafe() function, which keeps the msg.sender as the user address only and not the ODProxy contract.

File: ODProxy.sol
30:     bool _succeeded;
31:     (_succeeded, _response) = _target.delegatecall(_data);

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

3. The allowSafe() function gets called with the respective values. Before the execution begins, we enter the modifier safeAllowed() which takes in the _safe as parameter for which the permissions are being given to _usr.

File: ODSafeManager.sol
105:   function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
106:     address _owner = _safeData[_safe].owner;
107:     safeCan[_owner][_safe][_usr] = _ok;
108:     emit AllowSAFE(msg.sender, _safe, _usr, _ok);
109:   }

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

4. In the modifier, the following happens:

• On Line 50, the owner of the safe is extracted, which is the ODProxy contract
• On Line 51, we check if the msg.sender is not the owner. This condition is true since the msg.sender is the user's EOA address and not the ODProxy contract of the user due to the delegateCall that was made.
• On Line 51, the second condition evaluates to true as well since the user cannot give permissions to anyone (even themselves) in the first place.
• This causes both conditions to evaluate to true and we revert.

File: ODSafeManager.sol
49:   modifier safeAllowed(uint256 _safe) {
50:     address _owner = _safeData[_safe].owner;
51:     if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
52:     _;
53:   } 

Tools Used

Manual Review

Recommended Mitigation Steps

Consider either implementing another function in the ODProxy contract specifically to allow calling allowSafe() or use a different modifer that checks if the owner of the ODProxy is the caller.

Assessed type

call/delegatecall

Lines of code

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

Vulnerability details

Impact

A safe is associated with a unique safeHandler. This safeHandler can give permissions to addresses through the allowHandler() function which stores these approvals/disapprovals in the handlerCan mapping. Now let's say there is a safeHandler which has permissioned some addresses in the handlerCan mapping. When this safe is transferred to a new owner, the previous permissions that were added to the safeHandler are still attached to it without the new owner realizing it.

Proof of Concept

Here is the whole process:

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

1. Safe handler of a safe (owned by owner A) approves addresses [X,Y,Z] to the handlerCan mapping through the allowHandler() function.

File: ODSafeManager.sol
112:   function allowHandler(address _usr, uint256 _ok) external {
113:     handlerCan[msg.sender][_usr] = _ok;
114:     emit AllowHandler(msg.sender, _usr, _ok);
115:   }

2. Owner A now transfers the safe (associated with the safe handler) to an Owner B through the safeTransferFrom() function in the Vault721.sol contract which inherits the ERC721Enumerable contract (that inherits the ERC721 contract). During the call, the _afterTokenTransfer() hook is called (overridden in the Vault721.sol contract), which further calls the transferSAFEOwnership() function. In the function, we can see that the safe is transferred but the handlerCan mapping is not updated for the safeHandler, which means the old permissions (addresses [X,Y,Z]) for the safeHandler are still attached without the new owner B realizing it.

File: ODSafeManager.sol
136:   function transferSAFEOwnership(uint256 _safe, address _dst) external {
137:     require(msg.sender == address(vault721), 'SafeMngr: Only Vault721');
138: 
139: 
140:     if (_dst == address(0)) revert ZeroAddress();
141:     SAFEData memory _sData = _safeData[_safe];
142:     if (_dst == _sData.owner) revert AlreadySafeOwner();
143: 
144: 
145:     _usrSafes[_sData.owner].remove(_safe);
146:     _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
147: 
148: 
149:     _usrSafes[_dst].add(_safe);
150:     _usrSafesPerCollat[_dst][_sData.collateralType].add(_safe);
151: 
152:
153:     _safeData[_safe].owner = _dst;
154: 
155: 
156:     emit TransferSAFEOwnership(msg.sender, _safe, _dst);
157:   }

Tools Used

Manual Review

Recommended Mitigation Steps

It is not possible to remove the handlerCan permissions in the transferSAFEOwnership() function since it is stored in a mapping. The only solution to this would be to add another key field (named _owner) to update the safeHandler permissions correctly whenever a safe is transferred. We can see this pattern implemented for the safeCan mapping, which correctly updates the safe permissions on transfer.

Solution (use this mapping instead):

File: ODSafeManager.sol
41:   mapping(address _owner => (address _safeHandler => mapping(address _caller => uint256 _ok))) public handlerCan;

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The allowHandler() function is used to allow/disallow a handler address to manage a safe (as mentioned in the documentation here). The function is designed in such a way that a safeHandler (msg.sender) needs to make a call to the function allowHandler() with the address being allowed/disallowed as the parameter. But the safeHandler is not able call this function since it does not contain code in it's contract to make this call. This prevents a safeHandler from approving other addresses. Additionally, functions (quitSystem() and enterSystem()) in the ODSafeManager.sol contract that use the handlerAllowed() modifier always revert due to this issue.

Proof of Concept

Here is the whole process:

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

1. Let us understand the layout of the handlerCan mapping first.

• Description of mapping from documentation - Mapping of handler to a caller permissions
• The key field is the _safeHandler of a safe for which permissions are being given to the value field which is the caller address with the approval/disapproval represented by _ok

File: ODSafeManager.sol
41:   mapping(address _safeHandler => mapping(address _caller => uint256 _ok)) public handlerCan;

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

2. In the function allowHandler() below, the following happens:

• On Line 113, msg.sender is expected to be the safeHandler address calling the function so that the permissions are set correctly. This is where the issue arises since for safeHandler to be the msg.sender, the call needs to originate from the safeHandler contract, which does not have any function within it to make this call. Due to this, the allowHandler() function cannot be called and thus any functions in the ODSafeManager.sol that rely on the handlerAllowed modifier (or the handlerCan mapping) revert.

File: ODSafeManager.sol
112:   function allowHandler(address _usr, uint256 _ok) external {
113:     handlerCan[msg.sender][_usr] = _ok;
114:     emit AllowHandler(msg.sender, _usr, _ok);
115:   }

Tools Used

Manual Review

Recommended Mitigation Steps

The solution to this problem would be to implement an access controlled function within the SAFEHandler contract that allows the owner of the safe (since safeHandler is unique to a safe) to make an external call to the allowHandler() function.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

An owner of a safe can give permissions/approval of their safe to another address (let's say address B) through the allowSafe() function in the ODSafeManager.sol contract. But this other address (address B) also gets the power to approve other addresses for the owner's safe. This is a permissioning problem in the allowSafe() function (specifically the safeAllowed() modifier) which creates a security risk for the owner's safe.

This might look like a design choice initially but it has been confirmed as an issue with the sponsor:

Proof of Concept

Here is the whole process:

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

1. Owner of the safe gives permission/approval to address _usr (address 0x01 for example purposes) for uint256 _safe through the allowSafe() function.

• On Line 107, safeCan[_owner][_safe][_usr] = _ok; is set to any value other than 0 to represent approval.

File: ODSafeManager.sol
105:   function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
106:     address _owner = _safeData[_safe].owner;
107:     safeCan[_owner][_safe][_usr] = _ok;
108:     emit AllowSAFE(msg.sender, _safe, _usr, _ok);
109:   }

2. The previously set address _usr (address 0x01) can now call the allowSafe() function with parameters uint256 _safe which will be the owner's safe and another address _usr (address 0x02), which will give address 0x02 permissions/approval for the owner's safe.

3. This issue arises because of how the checks are evaluated in the safeAllowed() modifier. Here is what happens:

• On Line 50, the owner of the safe is extracted.
• On Line 51, there are two conditions present that are separated by the && operator.
• On Line 51, the first check evaluates to true, since the msg.sender (address 0x01) is not the owner of the safe
• On Line 51, the second check evaluates to false, since the msg.sender (address 0x01) was previously approved by the owner in step 1 above.
• Since true && false = false, we do not revert and this gives address 0x02 permissions to the owner's safe in the allowSafe() function.

File: ODSafeManager.sol
49:   modifier safeAllowed(uint256 _safe) {
50:     address _owner = _safeData[_safe].owner;
51:     if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
52:     _;
53:   } 

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a separate modifier for the allowSafe() function that only checks if the msg.sender is the owner. If true, then allow execution but if not then revert.

Solution:

File: ODSafeManager.sol
modifier onlySafeOwner(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner) revert SafeNotAllowed();
    _;
}

Assessed type

Access Control

Quality Assurance Report

Note: N = Non-Critical, L = Low-severity, R = Recommendation

Total Non-Critical issues: 21 instances across 7 issues

Total Low-severity issues: 4 instances across 4 issues

Total Recommendations: 6 instances across 4 issues

Total QA issues: 31 instances across 15 issues

[N-01] Public variable not used in external contracts can be marked private/internal

There are 3 instances of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L18C1-L20C34

File: src/contracts/proxies/Vault721.sol
18:   address public governor;
19:   IODSafeManager public safeManager;
20:   NFTRenderer public nftRenderer;

[N-02] Missing event emission for critical state changes

There are 13 instances of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L33C1-L35C4

File: Vault721.sol
34:   constructor(address _governor) ERC721('OpenDollar Vault', 'ODV') {
35:     governor = _governor;
36:     //@audit NC - missing event emission
37:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L104C1-L114C4

File: Vault721.sol
110:   function updateNftRenderer(
111:     address _nftRenderer,
112:     address _oracleRelayer,
113:     address _taxCollector,
114:     address _collateralJoinFactory
115:   ) external onlyGovernor nonZero(_oracleRelayer) nonZero(_taxCollector) nonZero(_collateralJoinFactory) {
116:     address _safeManager = address(safeManager);
117:     require(_safeManager != address(0));
118:     _setNftRenderer(_nftRenderer);
119:     nftRenderer.setImplementation(_safeManager, _oracleRelayer, _taxCollector, _collateralJoinFactory);
120:     //@audit NC - missing event emission
121:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L119C1-L121C4

File: Vault721.sol
127:   function updateContractURI(string memory _metaData) external onlyGovernor {
128:     contractMetaData = _metaData;
129:     //@audit NC - missing event emission
130:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L172C1-L175C1

File: Vault721.sol
183:   function _setSafeManager(address _safeManager) internal nonZero(_safeManager) {
184:     safeManager = IODSafeManager(_safeManager);
185:     //@audit NC - missing event emission
186:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L179C1-L181C4

File: Vault721.sol
191:   function _setNftRenderer(address _nftRenderer) internal nonZero(_nftRenderer) {
192:     nftRenderer = NFTRenderer(_nftRenderer);
193:     //@audit NC - missing event emission
194:   }

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

File: ODSafeManager.sol
65:   constructor(address _safeEngine, address _vault721) {
66:     safeEngine = _safeEngine.assertNonNull(); 
67:     vault721 = IVault721(_vault721);
68:     vault721.initializeManager();
69:     //@audit NC - missing event emission
70:   }

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

File: ODSafeManager.sol
265:   function addSAFE(uint256 _safe) external {
266:     SAFEData memory _sData = _safeData[_safe];
267:     _usrSafes[msg.sender].add(_safe);
268:     _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
269:     //@audit NC - missing event emission
270:   }

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

File: ODSafeManager.sol
273:   function removeSAFE(uint256 _safe) external safeAllowed(_safe) {
274:     SAFEData memory _sData = _safeData[_safe];
275:     _usrSafes[_sData.owner].remove(_safe);
276:     _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
277:     //@audit NC - missing event emission
278:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODProxy.sol#L14C1-L17C1

File: ODProxy.sol
14:   constructor(address _owner) {
15:     OWNER = _owner;
16:     //@audit NC - missing event emission
17:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L40C1-L62C4

File: CamelotRelayer.sol
40:   constructor(address _baseToken, address _quoteToken, uint32 _quotePeriod) {
41:     // camelotPair = ICamelotFactory(_CAMELOT_FACTORY).getPair(_baseToken, _quoteToken);
42:     camelotPair = IAlgebraFactory(_CAMELOT_FACTORY).poolByPair(_baseToken, _quoteToken);
43:     if (camelotPair == address(0)) revert CamelotRelayer_InvalidPool();
44: 
45:     address _token0 = ICamelotPair(camelotPair).token0();
46:     address _token1 = ICamelotPair(camelotPair).token1();
47: 
48:     // The factory validates that both token0 and token1 are desired baseToken and quoteTokens
49:     if (_token0 == _baseToken) {
50:       baseToken = _token0;
51:       quoteToken = _token1;
52:     } else {
53:       baseToken = _token1;
54:       quoteToken = _token0;
55:     }
56: 
57:     baseAmount = uint128(10 ** IERC20Metadata(_baseToken).decimals());
58:     multiplier = 18 - IERC20Metadata(_quoteToken).decimals(); 
59:     quotePeriod = _quotePeriod; 
60: 
61:     symbol = string(abi.encodePacked(IERC20Metadata(_baseToken).symbol(), ' / ', IERC20Metadata(_quoteToken).symbol()));
62:     //@audit NC - missing event emission
63:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/AccountingEngine.sol#L86

File: AccountingEngine.sol
086:   constructor(
087:     address _safeEngine,
088:     address _surplusAuctionHouse,
089:     address _debtAuctionHouse,
090:     AccountingEngineParams memory _accEngineParams
091:   ) Authorizable(msg.sender) validParams {
092:     safeEngine = ISAFEEngine(_safeEngine.assertNonNull());
093:     _setSurplusAuctionHouse(_surplusAuctionHouse);
094:     debtAuctionHouse = IDebtAuctionHouse(_debtAuctionHouse);
095: 
096:     lastSurplusTime = block.timestamp;
097: 
098:     _params = _accEngineParams;
099:     //@audit NC - missing event emission
100:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/AccountingEngine.sol#L247

File: AccountingEngine.sol
265:   function _onContractDisable() internal override {
266:     totalQueuedDebt = 0; 
267:     totalOnAuctionDebt = 0;
268:     disableTimestamp = block.timestamp;
269: 
270:     surplusAuctionHouse.disableContract();
271:     debtAuctionHouse.disableContract();
272: 
273:     uint256 _debtToSettle = Math.min(safeEngine.coinBalance(address(this)), safeEngine.debtBalance(address(this)));
274:     safeEngine.settleDebt(_debtToSettle);
275:     //@audit NC - missing event emission
276:   }

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/AccountingEngine.sol#L305

File: AccountingEngine.sol
326:   function _setSurplusAuctionHouse(address _surplusAuctionHouse) internal {
327:     if (address(surplusAuctionHouse) != address(0)) {
328:       safeEngine.denySAFEModification(address(surplusAuctionHouse));
329:     }
330:     surplusAuctionHouse = ISurplusAuctionHouse(_surplusAuctionHouse);
331:     safeEngine.approveSAFEModification(_surplusAuctionHouse);
332:     //@audit NC - Missing event emission
333:   }

[N-03] Cache variable early to prevent redundant caching and an extra SLOAD

There is 1 instance of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L94C1-L99C4

On Line 97, we can see _proxyRegistry[_proxy] being cached to _user. This caching is redundant since it could anyways be inlined on Line 98 directly. But we also observe _proxyRegistry[_proxy] on Line 96 in the require check. This is an extra SLOAD that could've been prevented if we perform the caching done on Line 97 before the require check on Line 96.

File: src/contracts/proxies/Vault721.sol
94:   function mint(address _proxy, uint256 _safeId) external {
95:     require(msg.sender == address(safeManager), 'V721: only safeManager');
96:     require(_proxyRegistry[_proxy] != address(0), 'V721: non-native proxy');
97:     address _user = _proxyRegistry[_proxy];
98:     _safeMint(_user, _safeId);
99:   }

Solution:

File: src/contracts/proxies/Vault721.sol
94:   function mint(address _proxy, uint256 _safeId) external {
95:     address _user = _proxyRegistry[_proxy];
96:     require(msg.sender == address(safeManager), 'V721: only safeManager');
97:     require(_user != address(0), 'V721: non-native proxy');
98:     _safeMint(_user, _safeId);
99:   }

[N-04] Remove redundant condition in _isNotProxy() function

There is 1 instance of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L154C1-L156C4

The condition ODProxy(_userRegistry[_user]).OWNER() != _user is redundant and the first condition itself is sufficient to determine whether a user has an ODProxy or not. There's never a situation where the second condition could evaluate to true. Look at the table below:

File: Vault721.sol
163:   function _isNotProxy(address _user) internal view returns (bool) {
164:     return _userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user;
165:   }

[N-05] Remove if (_dst == address(0)) revert ZeroAddress(); redundant check since it is already checked in _afterTokenTransfer() function

There is 1 instance of this issue:

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

Remove below check:

File: src/contracts/proxies/ODSafeManager.sol
139: if (_dst == address(0)) revert ZeroAddress();

Check already done before in _afterTokenTransfer() function:

File: Vault721.sol
206:       if (_isNotProxy(to)) {
207:         proxy = _build(to);
208:       } else {
209:         proxy = payable(_userRegistry[to]);
210:       }

[N-06] Function read() does not revert with "OLD!" as mentioned in comments

There is 1 instance of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L91C1-L101C4

Function does not revert with "OLD!" since there is no such revert message in the consult function.

File: CamelotRelayer.sol
092:   function read() external view returns (uint256 _result) {
093:     // This call may revert with 'OLD!' if the pool doesn't have enough cardinality or initialized history 
094:     (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);
095:     uint256 _quoteAmount = OracleLibrary.getQuoteAtTick({
096:       tick: _arithmeticMeanTick,
097:       baseAmount: baseAmount,
098:       baseToken: baseToken,
099:       quoteToken: quoteToken
100:     });
101:     _result = _parseResult(_quoteAmount);
102:   }

[N-07] Remove if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver(); redundant check

There is 1 instance of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/AccountingEngine.sol#L225

Remove check below:

225: if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();

Check already done before in the same function:

201: if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();

[L-01] Image field does not point to an image but the testnet website

There is 1 instance of this issue:

In the below string variable contractMetaData, we can observe the image field points as such "image": "https://app.opendollar.com/collectionImage.png". If we follow the link it leads us to the testnet website and not an image.

File: Vault721.sol
22:   string public contractMetaData =
23:     '{"name": "Open Dollar Vaults","description": "Tradable Vaults for the Open Dollar stablecoin protocol. Caution! Trading this NFT means trading the ownership of your Vault in the Open Dollar protocol and all of the assets/collateral inside each Vault.","image": "https://app.opendollar.com/collectionImage.png","external_link": "https://opendollar.com"}';

[L-02] Missing check in constructor to see if quotePeriod is not zero

There is 1 instance of this:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L59

File: CamelotRelayer.sol
59:     quotePeriod = _quotePeriod;

[L-03] Missing cardinality check in function read()

There is 1 instance of this issue:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L91C1-L101C4

On Line 93, the comment mentions that the read() function should revert with "OLD!" if the pool does not have enough cardinality or initialized history. But there is no check done for the cardinality, which can return an incorrect quote.

File: CamelotRelayer.sol
092:   function read() external view returns (uint256 _result) {
093:     // This call may revert with 'OLD!' if the pool doesn't have enough cardinality or initialized history
094:     (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);
095:     uint256 _quoteAmount = OracleLibrary.getQuoteAtTick({
096:       tick: _arithmeticMeanTick,
097:       baseAmount: baseAmount,
098:       baseToken: baseToken,
099:       quoteToken: quoteToken
100:     });
101:     _result = _parseResult(_quoteAmount);
102:   }

Solution:

File: CamelotRelayer.sol
69:   function read() external view returns (uint256 _result) {
70:     // If the pool doesn't have enough history return false
71:     if (OracleLibrary.getOldestObservationSecondsAgo(camelotPair) < quotePeriod) {
72:       return (0, false);
73:     }
74:     // Consult the query with a TWAP period of quotePeriod
75:     (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);
76:     // Calculate the quote amount
77:     uint256 _quoteAmount = OracleLibrary.getQuoteAtTick({
78:       tick: _arithmeticMeanTick,
79:       baseAmount: baseAmount,
80:       baseToken: baseToken,
81:       quoteToken: quoteToken
82:     });
83:     // Process the quote result to 18 decimal quote
84:     _result = _parseResult(_quoteAmount);
85:   }

[L-04] Rounding down in _exitCollateral() function can cause loss of precision leading to loss of funds for users

There is 1 instance of this issue:

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

In the function _exitCollateral(), users can experience loss of funds if the wad amount is smaller than the decimals representation in the denominator.

The below code snippet is from the CommonActions.sol contract is inherited in the BasicActions.sol contract. This _exitCollateral() function is called when the freeTokenCollateral() function in the BasicActions.sol contract is called. This function does the operation below based on the decimals of the ERC20 token being used. In case the numerator i.e. the _wad amount is smaller than the denominator, the final _weiAmount rounds down to zero. This can lead to loss of funds for the user who tries to exit with his collateral.

File: CommonActions.sol
118:     uint256 _weiAmount = _wad / 10 ** (18 - _decimals);
119:     __collateralJoin.exit(msg.sender, _weiAmount);

[R-01] Consider modifying the build() function which allows anyone to create an ODProxy for any user

There is 1 instance of this:

The build() function on Line 90 below allows anyone to create an ODProxy for any user. Although this is a convenient method, it should still implement some sort of approval mechanism where the caller can deploy an ODProxy only on the approval of the user (for whom the proxy is being created). That way the method is less prone to frontrunning attacks that could DOS the user's call (since the Proxy would already be created but user call fails when trying to build one).

File: Vault721.sol
81:   function build() external returns (address payable _proxy) {
82:     if (!_isNotProxy(msg.sender)) revert ProxyAlreadyExist();
83:     _proxy = _build(msg.sender);
84:   }
85: 
86:   /**
87:    * @dev allows user without an ODProxy to deploy a new ODProxy
88:    */
89:   //@audit Low - allows anyone to build an ODProxy for any user
90:   function build(address _user) external returns (address payable _proxy) {
91:     if (!_isNotProxy(_user)) revert ProxyAlreadyExist();
92:     _proxy = _build(_user);
93:   }

[R-02] Use user instead of usr in mappings to improve readability

There are 2 instances of this:

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

Instead of _usrSafes use _userSafes and _userSafesPerCollat instead of _usrSafesPerCollat

File: ODSafeManager.sol
32:   mapping(address _safeOwner => EnumerableSet.UintSet) private _usrSafes;
33:   /// @notice Mapping of user addresses to their enumerable set of safes per collateral type
34:   mapping(address _safeOwner => mapping(bytes32 _cType => EnumerableSet.UintSet)) private _usrSafesPerCollat; 

[R-03] Use safeAllow and handlerAllow instead of safeCan and handlerCan to better match the intention of the mappings

There are 2 instances of this:

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

These mappings represent the allowances the _owner gives to other addresses. To suit this intention of allowance, it would be better to rename the mappings to safeAllow and handlerAllow.

File: ODSafeManager.sol
40:   mapping(address _owner => mapping(uint256 _safeId => mapping(address _caller => uint256 _ok))) public safeCan;
41:   /// @inheritdoc IODSafeManager
42:   mapping(address _safeHandler => mapping(address _caller => uint256 _ok)) public handlerCan;

[R-04] Add brackets around 10 ** multiplier to improve code readability and provide clarity in which operation takes precedence first

There is 1 instance of this:

Instead of this:

File: CamelotRelayer.sol
104:   function _parseResult(uint256 _quoteResult) internal view returns (uint256 _result) {
105:     return _quoteResult * 10 ** multiplier;
106:   }

Use this:

File: CamelotRelayer.sol
104:   function _parseResult(uint256 _quoteResult) internal view returns (uint256 _result) {
105:     return _quoteResult * (10 ** multiplier);
106:   }