The function getEscrow() called by deposit() will execute the external call escrow.initialize() before updating the state variable escrows[user]. Consider updating the state before the external call to use the checks-effects-interactions pattern.
tx.origin can be manipulated by malicious contracts calling into BorrowController. Consider removing tx.origin for authorization and using a pattern where msg.sender is set during initalization and verified for later calls.
[03] block.timestamp strict equal check in DBR.accrueDueTokens()
Consider replacing lastUpdated[user] == block.timestamp, with lastUpdated[user] <= block.timestamp, to ensure that the current timestamp must be bigger the last update.
[04] Missing zero address checks for setters and constructor
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critial functionalities to the wrong address.
All the contracts in scope are floating the pragma version.
Consider locking the pragma to ensure that contracts do not accidentally get deployed using an outdated compiler version.
Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.
[08] Repeated validation statements can be converted into a function modifier to improve code reusability
[01] Lack of checks-effects-interactions in
Market.getEscrow()
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L245-L251
The function
getEscrow()
called bydeposit()
will execute the external callescrow.initialize()
before updating the state variableescrows[user]
. Consider updating the state before the external call to use the checks-effects-interactions pattern.[02] Usage of tx.origin
https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L47
tx.origin can be manipulated by malicious contracts calling into
BorrowController
. Consider removing tx.origin for authorization and using a pattern wheremsg.sender
is set during initalization and verified for later calls.[03] block.timestamp strict equal check in
DBR.accrueDueTokens()
https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L286
Consider replacing
lastUpdated[user] == block.timestamp
, withlastUpdated[user] <= block.timestamp
, to ensure that the current timestamp must be bigger the last update.[04] Missing zero address checks for setters and constructor
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L77-L80
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142
If a variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.
[05] Critical changes should use two-step procedure
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L118
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L124
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critial functionalities to the wrong address.
[06] Missing event for critical parameter changes
Adding events will facilitate offchain monitoring
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142
[07] Floating pragma
All the contracts in scope are floating the pragma version.
Consider locking the pragma to ensure that contracts do not accidentally get deployed using an outdated compiler version.
Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.
[08] Repeated validation statements can be converted into a function modifier to improve code reusability
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L74
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L149
[09] Interchangeable usage of uint and uint256
Following instance is using uint256
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L55
Following instance is using uint
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L69
Consider using only one identifier throughout the codebase, e.g. only uint or only uint256.
[10] Order of functions and events
The solidity documentation recommends the following order for functions:
constructor receive function (if exists) fallback function (if exists) external public internal private
Another recommendation from the style guide is related to declaring events before functions.
Consider adopting the following strategy throughout the codebase.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L245
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L258
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L258
[11] Replace magic numbers with constants to improve code readability
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L74-L76
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L563-L564
[12] Public functions not called by the contract should be declared external
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L591