From f6b0f06acc85087cdbc822cd1d56590f399181e9 Mon Sep 17 00:00:00 2001 From: Bryan Cole Date: Wed, 19 Jul 2023 12:09:22 -0700 Subject: [PATCH] feat(collateral-contract): adds a authorized sender role to block sender actions for unauthorized addresses Signed-off-by: Bryan Cole --- src/Collateral.sol | 35 ++++++++++++++++++------- test/Collateral.t.sol | 60 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/Collateral.sol b/src/Collateral.sol index 6c45dcc..997e6e4 100644 --- a/src/Collateral.sol +++ b/src/Collateral.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.18; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {AccessControlDefaultAdminRules} from "@openzeppelin/contracts/access/AccessControlDefaultAdminRules.sol"; import {TAPVerifier} from "./TAPVerifier.sol"; import {AllocationIDTracker} from "./AllocationIDTracker.sol"; import {IStaking} from "./IStaking.sol"; @@ -20,7 +21,7 @@ import {IStaking} from "./IStaking.sol"; * @notice This contract uses the `TAPVerifier` contract for recovering signer addresses * from `RAVs`. */ -contract Collateral { +contract Collateral is AccessControlDefaultAdminRules { using SafeERC20 for IERC20; struct CollateralAccount { @@ -34,6 +35,9 @@ contract Collateral { uint256 thawEndTimestamp; // Block number at which thawing period ends (zero if not thawing) } + // Role for depositing collateral + bytes32 public constant AUTHORIZED_SENDER = keccak256("AUTHORIZED_SENDER"); + // Stores how much collateral each sender has deposited for each receiver, as well as thawing information mapping(address sender => mapping(address reciever => CollateralAccount collateralAccount)) private collateralAccounts; @@ -159,8 +163,10 @@ contract Collateral { address tapVerifier_, address allocationIDTracker_, uint256 withdrawCollateralThawingPeriod_, - uint256 revokeSignerThawingPeriod_ - ) { + uint256 revokeSignerThawingPeriod_, + uint48 accessControlInitialDelay, + address accessControlInitialDefaultAdmin + ) AccessControlDefaultAdminRules(accessControlInitialDelay, accessControlInitialDefaultAdmin) { collateralToken = IERC20(collateralToken_); staking = IStaking(staking_); tapVerifier = TAPVerifier(tapVerifier_); @@ -184,7 +190,10 @@ contract Collateral { * @notice The collateral must be approved for transfer by the sender. * @notice REVERT: this function will revert if the collateral transfer fails. */ - function deposit(address receiver, uint256 amount) external { + function deposit( + address receiver, + uint256 amount + ) external onlyRole(AUTHORIZED_SENDER) { collateralAccounts[msg.sender][receiver].balance += amount; collateralToken.safeTransferFrom(msg.sender, address(this), amount); emit Deposit(msg.sender, receiver, amount); @@ -198,7 +207,10 @@ contract Collateral { * - InsufficientCollateral: if the sender receiver collateral account does * not have enough collateral (greater than `amount`) */ - function thaw(address receiver, uint256 amount) external { + function thaw( + address receiver, + uint256 amount + ) external onlyRole(AUTHORIZED_SENDER) { CollateralAccount storage account = collateralAccounts[msg.sender][ receiver ]; @@ -236,7 +248,7 @@ contract Collateral { * - CollateralStillThawing: ThawEndTimestamp has not been reached * for collateral currently thawing */ - function withdraw(address receiver) external { + function withdraw(address receiver) external onlyRole(AUTHORIZED_SENDER) { CollateralAccount storage account = collateralAccounts[msg.sender][ receiver ]; @@ -273,7 +285,10 @@ contract Collateral { * - SignerAlreadyAuthorized: Signer is currently authorized for a sender * - InvalidSignerProof: The provided signer proof is invalid */ - function authorizeSigner(address signer, bytes calldata proof) external { + function authorizeSigner( + address signer, + bytes calldata proof + ) external onlyRole(AUTHORIZED_SENDER) { if (authorizedSigners[signer].sender != address(0)) { revert SignerAlreadyAuthorized( signer, @@ -295,7 +310,7 @@ contract Collateral { * - SignerNotAuthorizedBySender: The provided signer is either not authorized or * authorized by a different sender */ - function thawSigner(address signer) external { + function thawSigner(address signer) external onlyRole(AUTHORIZED_SENDER) { SenderAuthorization storage authorization = authorizedSigners[signer]; if (authorization.sender != msg.sender) { @@ -325,7 +340,9 @@ contract Collateral { * - SignerStillThawing: ThawEndTimestamp has not been reached * for provided signer */ - function revokeAuthorizedSigner(address signer) external { + function revokeAuthorizedSigner( + address signer + ) external onlyRole(AUTHORIZED_SENDER) { SenderAuthorization storage authorization = authorizedSigners[signer]; if (authorization.sender != msg.sender) { diff --git a/test/Collateral.t.sol b/test/Collateral.t.sol index 5656200..c515df2 100644 --- a/test/Collateral.t.sol +++ b/test/Collateral.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.18; import "forge-std/Test.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {TAPVerifier} from "../src/TAPVerifier.sol"; import {Collateral} from "../src/Collateral.sol"; import {MockERC20Token} from "./MockERC20Token.sol"; @@ -46,7 +47,15 @@ contract CollateralContractTest is Test { assert(mockERC20.transfer(SENDER_ADDRESS, 10000000)); collateralContract = - new Collateral(address(mockERC20), address(staking), address(tap_verifier), address(allocationIDTracker), WITHDRAW_COLLATERAL_FREEZE_PERIOD, REVOKE_SIGNER_FREEZE_PERIOD); + new Collateral( + address(mockERC20), + address(staking), + address(tap_verifier), + address(allocationIDTracker), + WITHDRAW_COLLATERAL_FREEZE_PERIOD, + REVOKE_SIGNER_FREEZE_PERIOD, + 3 days, + address(this)); // Approve staking contract to transfer tokens from the collateral contract collateralContract.approveAll(); @@ -88,7 +97,7 @@ contract CollateralContractTest is Test { } function testDepositFunds() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); vm.prank(SENDER_ADDRESS); uint256 depositedAmount = collateralContract.getCollateralAmount(SENDER_ADDRESS, receiverAddress); @@ -97,7 +106,7 @@ contract CollateralContractTest is Test { } function testWithdrawFundsAfterFreezePeriod() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); // Sets msg.sender address for next contract calls until stop is called vm.startPrank(SENDER_ADDRESS); @@ -121,7 +130,7 @@ contract CollateralContractTest is Test { } function testRedeemRAVSignedByAuthorizedSigner() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); uint256 remainingCollateral = collateralContract.getCollateralAmount(SENDER_ADDRESS, receiverAddress); assertEq(remainingCollateral, COLLATERAL_AMOUNT, "Incorrect remaining collateral"); @@ -160,14 +169,14 @@ contract CollateralContractTest is Test { } function testGetCollateralAmount() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); uint256 depositedAmount = collateralContract.getCollateralAmount(SENDER_ADDRESS, receiverAddress); assertEq(depositedAmount, COLLATERAL_AMOUNT, "Incorrect deposited amount"); } function testMultipleThawRequests() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); uint256 partialCollateralAmount = COLLATERAL_AMOUNT / 10; uint256 partialFreezePeriod = WITHDRAW_COLLATERAL_FREEZE_PERIOD / 10; uint256 expectedThawEnd = 0; @@ -205,7 +214,7 @@ contract CollateralContractTest is Test { // test that the contract reverts when allocation ID is used more than once function testRevertOnDuplicateAllocationID() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); uint256 remainingCollateral = collateralContract.getCollateralAmount(SENDER_ADDRESS, receiverAddress); assertEq(remainingCollateral, COLLATERAL_AMOUNT, "Incorrect remaining collateral"); @@ -263,7 +272,7 @@ contract CollateralContractTest is Test { // create additional sender address to test that the contract does not revert when redeeming same allocation ID with a different sender address secondSenderAddress = address(0xa789); assert(mockERC20.transfer(secondSenderAddress, 10000000)); - depositCollateral(secondSenderAddress, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(secondSenderAddress, receiverAddress, COLLATERAL_AMOUNT, true); // should not revert when redeeming same allocationID with a different sender authorizeSignerWithProof(secondSenderAddress, authorizedSignerPrivateKeys[1], authorizedsigners[1]); @@ -291,7 +300,7 @@ contract CollateralContractTest is Test { } function testRevokeAuthorizedSigner() public { - depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT); + depositCollateral(SENDER_ADDRESS, receiverAddress, COLLATERAL_AMOUNT, true); authorizeSignerWithProof(SENDER_ADDRESS, authorizedSignerPrivateKeys[0], authorizedsigners[0]); vm.prank(SENDER_ADDRESS); @@ -336,6 +345,34 @@ contract CollateralContractTest is Test { ); } + function testAccessControlledFunctions() public { + string memory expectedErrorMessage = string( + abi.encodePacked( + "AccessControl: account ", + Strings.toHexString(SENDER_ADDRESS), + " is missing role ", + Strings.toHexString(uint256(collateralContract.AUTHORIZED_SENDER()), 32) + ) + ); + // Call functions as sender (who currently has no role and is not authorized to call these functions) + vm.startPrank(SENDER_ADDRESS); + + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.deposit(receiverAddress, 10); + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.thaw(receiverAddress, 10); + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.withdraw(receiverAddress); + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.authorizeSigner(authorizedsigners[0], bytes("test")); + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.thawSigner(authorizedsigners[0]); + vm.expectRevert(bytes(expectedErrorMessage)); + collateralContract.revokeAuthorizedSigner(authorizedsigners[0]); + + vm.stopPrank(); + } + function authorizeSignerWithProof(address sender, uint256 signerPivateKey, address signer) private { bytes memory authSignerAuthorizesSenderProof = createAuthorizedSignerProof(sender, signerPivateKey); @@ -367,7 +404,10 @@ contract CollateralContractTest is Test { return abi.encodePacked(r, s, v); } - function depositCollateral(address sender, address receiver, uint256 amount) public { + function depositCollateral(address sender, address receiver, uint256 amount, bool grantRole) public { + if( grantRole && !collateralContract.hasRole(collateralContract.AUTHORIZED_SENDER(), sender) ) { + collateralContract.grantRole(collateralContract.AUTHORIZED_SENDER(), sender); + } // Sets msg.sender address for next contract calls until stop is called vm.startPrank(sender); // Approve the collateral contract to transfer tokens from the sender