Skip to content

Commit

Permalink
feat: add onlyEVCAccount modifier
Browse files Browse the repository at this point in the history
  • Loading branch information
@kasperpawlowski
kasperpawlowski committed Sep 9, 2024
1 parent 38e459d commit e64ca4a
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 25 deletions.
35 changes: 25 additions & 10 deletions src/utils/EVCUtil.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ abstract contract EVCUtil {
_;
}

/// @notice Ensures a standard authentication path on the EVC allowing the account owner or any EVC account.
/// @dev This modifier checks if the caller is the EVC and if so, verifies the execution context.
/// It reverts if the operator is authenticated, control collateral is in progress, or checks are in progress.
/// @dev This modifier must not be used on functions utilized by liquidation flows, i.e. transfer or withdraw.
/// @dev This modifier must not be used on checkAccountStatus and checkVaultStatus functions.
/// @dev This modifier can be used on access controlled functions to prevent non-standard authentication paths on
/// the EVC.
modifier onlyEVCAccount() virtual {
_onlyEVCAccount(false);
_;
}

/// @notice Ensures a standard authentication path on the EVC.
/// @dev This modifier checks if the caller is the EVC and if so, verifies the execution context.
/// It reverts if the operator is authenticated, control collateral is in progress, or checks are in progress.
Expand All @@ -59,7 +71,7 @@ abstract contract EVCUtil {
/// @dev This modifier can be used on access controlled functions to prevent non-standard authentication paths on
/// the EVC.
modifier onlyEVCAccountOwner() virtual {
_onlyEVCAccountOwner();
_onlyEVCAccount(true);
_;
}

Expand Down Expand Up @@ -159,24 +171,27 @@ abstract contract EVCUtil {
}
}

/// @notice Ensures that the function is called only by the EVC account owner
/// @notice Ensures that the function is called only by the EVC account owner or any EVC account
/// @dev This function checks if the caller is the EVC and if so, verifies that the execution context is not in a
/// special state (operator authenticated, collateral control in progress, or checks in progress). If the owner was
/// already registered on the EVC, it verifies that the onBehalfOfAccount is the owner.
/// @dev Reverts if the caller is not the EVC or if the execution context is in a special state.
function _onlyEVCAccountOwner() internal view {
/// special state (operator authenticated, collateral control in progress, or checks in progress). If
/// onlyAccountOwner is true and the owner was already registered on the EVC, it verifies that the onBehalfOfAccount
/// is the owner. If onlyAccountOwner is false, it allows any EVC account to call the function.
/// @param onlyAccountOwner If true, only allows the account owner; if false, allows any EVC account
function _onlyEVCAccount(bool onlyAccountOwner) internal view {
if (msg.sender == address(evc)) {
EC ec = EC.wrap(evc.getRawExecutionContext());

if (ec.isOperatorAuthenticated() || ec.isControlCollateralInProgress() || ec.areChecksInProgress()) {
revert NotAuthorized();
}

address onBehalfOfAccount = ec.getOnBehalfOfAccount();
address owner = evc.getAccountOwner(onBehalfOfAccount);
if (onlyAccountOwner) {
address onBehalfOfAccount = ec.getOnBehalfOfAccount();
address owner = evc.getAccountOwner(onBehalfOfAccount);

if (owner != address(0) && owner != onBehalfOfAccount) {
revert NotAuthorized();
if (owner != address(0) && owner != onBehalfOfAccount) {
revert NotAuthorized();
}
}
}
}
Expand Down
14 changes: 6 additions & 8 deletions test/invariants/EthereumVaultConnector.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,9 @@ contract EthereumVaultConnectorHandler is EthereumVaultConnectorScribble, Test {
return;
}

function batchSimulation(BatchItem[] calldata)
function batchSimulation(
BatchItem[] calldata
)
public
payable
override
Expand All @@ -257,13 +259,9 @@ contract EthereumVaultConnectorHandler is EthereumVaultConnectorScribble, Test {
accountControllers[account].firstElement = firstElementCache;
}

function checkAccountStatusInternal(address account)
internal
view
virtual
override
returns (bool isValid, bytes memory result)
{
function checkAccountStatusInternal(
address account
) internal view virtual override returns (bool isValid, bytes memory result) {
SetStorage storage accountControllersStorage = accountControllers[account];
uint256 numOfControllers = accountControllersStorage.numElements;

Expand Down
42 changes: 35 additions & 7 deletions test/unit/EVCUtil/EVCUtil.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,9 @@ contract EVCClient is EVCUtil {
return param;
}

function calledThroughEVCPayableBytes(bytes calldata param)
external
payable
callThroughEVC
returns (bytes memory)
{
function calledThroughEVCPayableBytes(
bytes calldata param
) external payable callThroughEVC returns (bytes memory) {
require(msg.sender == address(evc), "Not EVC");
return param;
}
Expand All @@ -41,6 +38,10 @@ contract EVCClient is EVCUtil {
// do nothing
}

function calledByEVCAccount() external onlyEVCAccount {
// do nothing
}

function calledByEVCAccountOwner() external onlyEVCAccountOwner {
// do nothing
}
Expand Down Expand Up @@ -187,38 +188,57 @@ contract EVCUtilTest is Test {
evcClient.calledByEVCWithChecksInProgress();
}

function test_calledByEVCAccountOwner(address caller, uint8 id) external {
function test_calledByEVCAccount_calledByEVCAccountOwner(address caller, uint8 id) external {
vm.assume(!evc.haveCommonOwner(caller, address(0)) && !evc.haveCommonOwner(caller, address(evc)));
vm.assume(id != 0);

// msg.sender is not EVC
evc.setOnBehalfOfAccount(address(0));
vm.prank(caller);
evcClient.calledByEVCAccount();

vm.prank(caller);
evcClient.calledByEVCAccountOwner();

// msg.sender is EVC and operator is authenticated
evc.setOperatorAuthenticated(true);
vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccount();

vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccountOwner();

// msg.sender is EVC and control collateral is in progress
evc.setOperatorAuthenticated(false);
evc.setControlCollateralInProgress(true);
vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccount();

vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccountOwner();

// msg.sender is EVC and checks are in progress
evc.setControlCollateralInProgress(false);
evc.setChecksInProgress(true);
vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccount();

vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccountOwner();

// msg.sender is EVC, the owner is not registered yet
evc.setChecksInProgress(false);
evc.setOnBehalfOfAccount(caller);
assertEq(evc.getAccountOwner(caller), address(0));
vm.prank(address(evc));
evcClient.calledByEVCAccount();

assertEq(evc.getAccountOwner(caller), address(0));
vm.prank(address(evc));
evcClient.calledByEVCAccountOwner();
Expand All @@ -227,13 +247,21 @@ contract EVCUtilTest is Test {
evc.setOnBehalfOfAccount(address(0));
vm.prank(caller);
evc.call(address(0), caller, 0, "");
assertEq(evc.getAccountOwner(caller), caller);
evc.setOnBehalfOfAccount(caller);
vm.prank(address(evc));
evcClient.calledByEVCAccount();

assertEq(evc.getAccountOwner(caller), caller);
evc.setOnBehalfOfAccount(caller);
vm.prank(address(evc));
evcClient.calledByEVCAccountOwner();

// msg.sender is EVC, the owner is registered but the authenticated account is not the owner
evc.setOnBehalfOfAccount(address(uint160(uint160(caller) ^ id)));
vm.prank(address(evc));
evcClient.calledByEVCAccount();

vm.prank(address(evc));
vm.expectRevert(abi.encodeWithSelector(EVCUtil.NotAuthorized.selector));
evcClient.calledByEVCAccountOwner();
Expand Down

0 comments on commit e64ca4a

Please sign in to comment.