DittoETH

Summary

The onlyDiamond modifier in the Modifiers contract is designed to restrict access to certain functions to the diamond contract. It does this by checking if msg.sender is equal to a specific diamond contract's address. However, the current implementation of onlyDiamond checks if msg.sender is equal to address(this), which is the address of the contract itself. This discrepancy between the intended behavior and the actual implementation.

Vulnerability Details

In the context of createForcedBid(), _performForcedBid() and liquidate() functions, the incorrect implementation of the onlyDiamond modifier could lead to restricted access.

Impact

The onlyDiamond modifier is incorrectly restricting access to the contract itself, _performForcedBid() might not be able to call createForcedBid() if it's intended to be called by diamond contract.

The liquidate() method calls the _performForcedBid() function, which in turn attempts to call the createForcedBid() function. If the createForcedBid() function is intended to be called by a diamond contract and is protected by the onlyDiamond modifier, the incorrect implementation of the onlyDiamond modifier could prevent the createForcedBid() function from being called.

This is because the onlyDiamond modifier is checking if msg.sender is equal to address(this), which is the address of the contract itself, not the diamond contract. If the createForcedBid() function cannot be called, the liquidate() method may not function as intended, leading to a failure in the liquidation process.

Tools Used

manual review

Recommendations

The Docs:
"onlyDiamond: Checks that the caller is the diamond contract. This is to give privileged access to certain external functions."

Bad
modifier onlyDiamond() {
if (msg.sender != address(this)) revert Errors.NotDiamond();
_;
}

Mitigation recommendation:

modifier onlyDiamond1() {
if (msg.sender != diamond) {
revert NotDiamond();
}
_;
}