Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit fixes #108

Open
wants to merge 4 commits into
base: compliance
Choose a base branch
from
Open

Audit fixes #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c23b6f
remove setWithdrawer() and unused code(fix #2, #5, #6, #7, #8)
0xvv Jan 15, 2025
c105e21
added missing events & getters
0xvv Jan 17, 2025
be9bf0e
simplify length checks
0xvv Jan 17, 2025
692efb0
prevent dispatching if validator owner is sanctioned
0xvv Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added .yarn/install-state.gz
View file
Open in desktop
Binary file not shown.
72 changes: 25 additions & 47 deletions src/contracts/StakingContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,6 @@ contract StakingContract {
error AddressSanctioned(address sanctionedAccount);
error AddressBlocked(address blockedAccount);

struct ValidatorAllocationCache {
bool used;
uint8 operatorIndex;
uint32 funded;
uint32 toDeposit;
uint32 available;
}

event Deposit(address indexed caller, address indexed withdrawer, bytes publicKey, bytes signature);
event ValidatorKeysAdded(uint256 indexed operatorIndex, bytes publicKeys, bytes signatures);
event ValidatorKeyRemoved(uint256 indexed operatorIndex, bytes publicKey);
Expand All @@ -75,9 +67,10 @@ contract StakingContract {
event ChangedOperatorAddresses(uint256 operatorIndex, address operatorAddress, address feeRecipientAddress);
event DeactivatedOperator(uint256 _operatorIndex);
event ActivatedOperator(uint256 _operatorIndex);
event SetWithdrawerCustomizationStatus(bool _status);
event ExitRequest(address caller, bytes pubkey);
event ValidatorsEdited(uint256 blockNumber);
event NewSanctionsOracle(address sanctionsOracle);
event BeginOwnershipTransfer(address indexed previousAdmin, address indexed newAdmin);

/// @notice Ensures an initialisation call has been called only once per _version value
/// @param _version The current initialisation value
Expand Down Expand Up @@ -199,18 +192,19 @@ contract StakingContract {
StakingContractStorageLib.setOperatorCommissionLimit(operatorCommissionLimitBPS);
}

/// @notice Changes the behavior of the withdrawer customization logic
/// @param _enabled True to allow users to customize the withdrawer
function setWithdrawerCustomizationEnabled(bool _enabled) external onlyAdmin {
StakingContractStorageLib.setWithdrawerCustomizationEnabled(_enabled);
emit SetWithdrawerCustomizationStatus(_enabled);
}

/// @notice Changes the sanctions oracle address
/// @param _sanctionsOracle New sanctions oracle address
/// @dev If the address is address(0), the sanctions oracle checks are skipped
function setSanctionsOracle(address _sanctionsOracle) external onlyAdmin {
StakingContractStorageLib.setSanctionsOracle(_sanctionsOracle);
emit NewSanctionsOracle(_sanctionsOracle);
}

/// @notice Get the sanctions oracle address
/// @notice If the address is address(0), the sanctions oracle checks are skipped
/// @return sanctionsOracle The sanctions oracle address
function getSanctionsOracle() external view returns (address) {
return StakingContractStorageLib.getSanctionsOracle();
}

/// @notice Retrieve system admin
Expand Down Expand Up @@ -274,9 +268,20 @@ contract StakingContract {

/// @notice Retrieve withdrawer of public key root
/// @notice In case the validator is not enabled, it will return address(0)
/// @notice In case the owner of the validator is sanctioned, it will revert
/// @param _publicKeyRoot Hash of the public key
function getWithdrawerFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (address) {
return _getWithdrawer(_publicKeyRoot);
address withdrawer = _getWithdrawer(_publicKeyRoot);
if (withdrawer == address(0)) {
return address(0);
}
address sanctionsOracle = StakingContractStorageLib.getSanctionsOracle();
if (sanctionsOracle != address(0)) {
if (ISanctionsOracle(sanctionsOracle).isSanctioned(withdrawer)) {
revert AddressSanctioned(withdrawer);
}
}
return withdrawer;
}

/// @notice Retrieve whether the validator exit has been requested
Expand Down Expand Up @@ -375,6 +380,7 @@ contract StakingContract {
/// @param _newAdmin New Administrator address
function transferOwnership(address _newAdmin) external onlyAdmin {
StakingContractStorageLib.setPendingAdmin(_newAdmin);
emit BeginOwnershipTransfer(msg.sender, _newAdmin);
}

/// @notice New admin must accept its role by calling this method
Expand Down Expand Up @@ -433,27 +439,6 @@ contract StakingContract {
emit ChangedOperatorAddresses(_operatorIndex, _operatorAddress, _feeRecipientAddress);
}

/// @notice Set withdrawer for public key
/// @dev Only callable by current public key withdrawer
/// @param _publicKey Public key to change withdrawer
/// @param _newWithdrawer New withdrawer address
function setWithdrawer(bytes calldata _publicKey, address _newWithdrawer) external {
if (!StakingContractStorageLib.getWithdrawerCustomizationEnabled()) {
revert Forbidden();
}
_checkAddress(_newWithdrawer);
bytes32 pubkeyRoot = _getPubKeyRoot(_publicKey);
StakingContractStorageLib.WithdrawersSlot storage withdrawers = StakingContractStorageLib.getWithdrawers();

if (withdrawers.value[pubkeyRoot] != msg.sender) {
revert Unauthorized();
}

emit ChangedWithdrawer(_publicKey, _newWithdrawer);

withdrawers.value[pubkeyRoot] = _newWithdrawer;
}

/// @notice Set operator staking limits
/// @dev Only callable by admin
/// @dev Limit should not exceed the validator key count of the operator
Expand Down Expand Up @@ -547,11 +532,11 @@ contract StakingContract {
revert InvalidArgument();
}

if (_publicKeys.length % PUBLIC_KEY_LENGTH != 0 || _publicKeys.length / PUBLIC_KEY_LENGTH != _keyCount) {
if (_publicKeys.length != PUBLIC_KEY_LENGTH * _keyCount) {
revert InvalidPublicKeys();
}

if (_signatures.length % SIGNATURE_LENGTH != 0 || _signatures.length / SIGNATURE_LENGTH != _keyCount) {
if (_signatures.length != SIGNATURE_LENGTH * _keyCount) {
revert InvalidSignatures();
}

Expand Down Expand Up @@ -967,13 +952,6 @@ contract StakingContract {
_depositOnOneOperator(depositCount, totalAvailableValidators);
}

function _min(uint256 _a, uint256 _b) internal pure returns (uint256) {
if (_a < _b) {
return _a;
}
return _b;
}

/// @notice Internal utility to compute the receiver deterministic address
/// @param _publicKey Public Key assigned to the receiver
/// @param _prefix Prefix used to generate multiple receivers per public key
Expand Down
15 changes: 0 additions & 15 deletions src/contracts/libs/StakingContractStorageLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,21 +315,6 @@ library StakingContractStorageLib {
===========================================
=========================================*/

bytes32 internal constant WITHDRAWER_CUSTOMIZATION_ENABLED_SLOT =
keccak256("StakingContract.withdrawerCustomizationEnabled");

function getWithdrawerCustomizationEnabled() internal view returns (bool) {
return getBool(WITHDRAWER_CUSTOMIZATION_ENABLED_SLOT);
}

function setWithdrawerCustomizationEnabled(bool _enabled) internal {
setBool(WITHDRAWER_CUSTOMIZATION_ENABLED_SLOT, _enabled);
}

/* ========================================
===========================================
=========================================*/

bytes32 internal constant EXIT_REQUEST_MAPPING_SLOT =
bytes32(uint256(keccak256("StakingContract.exitRequest")) - 1);

Expand Down
116 changes: 44 additions & 72 deletions src/test/StakingContract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,16 @@ contract StakingContractTest is Test {
assertEq(stakingContract.getAdmin(), admin);
}

event BeginOwnershipTransfer(address indexed previousAdmin, address indexed newAdmin);
event ChangedAdmin(address newAdmin);

function testSetAdmin(address newAdmin) public {
assertEq(stakingContract.getAdmin(), admin);

// Start ownership transfer process.
vm.startPrank(admin);
vm.expectEmit(true, true, true, true);
emit BeginOwnershipTransfer(admin, newAdmin);
stakingContract.transferOwnership(newAdmin);
vm.stopPrank();
// At this point, the old admin is still in charge.
Expand Down Expand Up @@ -787,78 +790,6 @@ contract StakingContractTest is Test {
vm.stopPrank();
}

event SetWithdrawerCustomizationStatus(bool _status);

function testSetWithdrawer(address user, address anotherUser) public {
vm.assume(user != address(depositContract) && anotherUser != address(depositContract));
vm.assume(anotherUser != address(0));
vm.deal(user, 32 * 3 ether);

vm.startPrank(user);
(bool _success, ) = address(stakingContract).call{value: 32 * 3 ether}("");
assert(_success == true);
vm.stopPrank();

assertEq(user.balance, 0);

bytes memory pk = PUBKEY_1;

assertEq(stakingContract.getWithdrawer(pk), user);

vm.startPrank(admin);
vm.expectEmit(true, true, true, true);
emit SetWithdrawerCustomizationStatus(true);
stakingContract.setWithdrawerCustomizationEnabled(true);
vm.stopPrank();

vm.startPrank(user);
stakingContract.setWithdrawer(pk, anotherUser);
vm.stopPrank();
}

function testSetWithdrawerForbidden(address user, address anotherUser) public {
vm.assume(user != address(depositContract) && anotherUser != address(depositContract));
vm.deal(user, 32 * 3 ether);

vm.startPrank(user);
(bool _success, ) = address(stakingContract).call{value: 32 * 3 ether}("");
assert(_success == true);
vm.stopPrank();

assertEq(user.balance, 0);

assertEq(stakingContract.getWithdrawer(PUBKEY_1), user);

vm.startPrank(user);
vm.expectRevert(abi.encodeWithSignature("Forbidden()"));
stakingContract.setWithdrawer(PUBKEY_1, anotherUser);
vm.stopPrank();
}

function testSetWithdrawerUnauthorized(address user, address anotherUser) public {
vm.assume(user != address(depositContract) && anotherUser != address(depositContract));
vm.assume(anotherUser != address(0) && anotherUser != user);
vm.deal(user, 32 * 3 ether);

vm.startPrank(user);
(bool _success, ) = address(stakingContract).call{value: 32 * 3 ether}("");
assert(_success == true);
vm.stopPrank();

assertEq(user.balance, 0);

vm.startPrank(admin);
stakingContract.setWithdrawerCustomizationEnabled(true);
vm.stopPrank();

assertEq(stakingContract.getWithdrawer(PUBKEY_1), user);

vm.startPrank(anotherUser);
vm.expectRevert(abi.encodeWithSignature("Unauthorized()"));
stakingContract.setWithdrawer(PUBKEY_1, anotherUser);
vm.stopPrank();
}

event ChangedTreasury(address newTreasury);

function testSetTreasury(address newTreasury) public {
Expand Down Expand Up @@ -2055,6 +1986,18 @@ contract StakingContractBehindProxyTest is Test {
assert(deactivated == false);
}

event NewSanctionsOracle(address);

function test_setSanctionsOracle() public {
assertEq(stakingContract.getSanctionsOracle(), address(0));
vm.startPrank(admin);
vm.expectEmit(true, true, true, true);
emit NewSanctionsOracle(address(oracle));
stakingContract.setSanctionsOracle(address(oracle));
vm.stopPrank();
assertEq(stakingContract.getSanctionsOracle(), address(oracle));
}

function test_deposit_withsanctions_senderSanctioned(address user) public {
assumeAddress(user);
oracle.setSanction(user, true);
Expand Down Expand Up @@ -3291,4 +3234,33 @@ contract StakingContractBehindProxyTest is Test {
vm.prank(admin);
stakingContract.blockAccount(bob, PUBKEY_2);
}

error AddressSanctioned(address addr);

function test_withdraw_from_recipient_owner_sanctioned() public {
vm.prank(admin);
stakingContract.setSanctionsOracle(address(oracle));
assertFalse(oracle.isSanctioned(bob));

vm.deal(bob, 32 ether);

vm.prank(bob);
stakingContract.deposit{value: 32 ether}();

address clfr = stakingContract.getCLFeeRecipient(PUBKEY_1);
vm.deal(clfr, 1 ether);

vm.prank(bob);
// First withdrawal deploy the recipient such that afterwards it's possible to trigger
// the withdrawal from the recipient directly
stakingContract.withdrawCLFee(PUBKEY_1);

oracle.setSanction(bob, true);

vm.deal(clfr, 1 ether);

vm.prank(bob);
vm.expectRevert(abi.encodeWithSignature("AddressSanctioned(address)", bob));
IFeeRecipient(clfr).withdraw();
}
}
Loading