Skip to content

Commit 9805925

Browse files
feat: Add security improvements based on Slither audit
Security enhancements implemented: - Add zero-address validation in constructor, setTransferAgent, and withdrawFees - Add RequiredSignaturesUpdated event emission in RTAProxy - Fix parameter naming conventions (remove underscore prefixes) Impact: - Prevents deployment with invalid transfer agent address - Ensures recipient validation for fee withdrawals - Improves event transparency for multi-sig operations - Follows Solidity naming best practices Testing: - All 63 tests passing - Slither re-scan shows all Priority 1 issues resolved - Minimal gas impact (<1000 gas for setup operations) This commit addresses all critical findings from the security audit and prepares the contracts for professional third-party review.
1 parent ca6cec2 commit 9805925

File tree

3 files changed

+24
-13
lines changed

3 files changed

+24
-13
lines changed

contracts/ERC1450.sol

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ contract ERC1450 is IERC1450, IERC20Metadata, ERC165, Ownable, ReentrancyGuard {
9292
address initialOwner,
9393
address initialTransferAgent
9494
) Ownable(initialOwner) {
95+
require(initialTransferAgent != address(0), "ERC1450: Invalid transfer agent");
9596
_name = name_;
9697
_symbol = symbol_;
9798
_decimals = decimals_;
@@ -176,6 +177,8 @@ contract ERC1450 is IERC1450, IERC20Metadata, ERC165, Ownable, ReentrancyGuard {
176177
}
177178

178179
function setTransferAgent(address newTransferAgent) external override {
180+
require(newTransferAgent != address(0), "ERC1450: Invalid transfer agent");
181+
179182
if (_transferAgentLocked) {
180183
revert ERC1450TransferAgentLocked();
181184
}
@@ -360,22 +363,24 @@ contract ERC1450 is IERC1450, IERC20Metadata, ERC165, Ownable, ReentrancyGuard {
360363
}
361364

362365
function setFeeParameters(
363-
uint8 _feeType,
364-
uint256 _feeValue,
365-
address[] calldata _acceptedTokens
366+
uint8 newFeeType,
367+
uint256 newFeeValue,
368+
address[] calldata newAcceptedTokens
366369
) external override onlyTransferAgent {
367-
feeType = _feeType;
368-
feeValue = _feeValue;
369-
acceptedFeeTokens = _acceptedTokens;
370+
feeType = newFeeType;
371+
feeValue = newFeeValue;
372+
acceptedFeeTokens = newAcceptedTokens;
370373

371-
emit FeeParametersUpdated(_feeType, _feeValue, _acceptedTokens);
374+
emit FeeParametersUpdated(newFeeType, newFeeValue, newAcceptedTokens);
372375
}
373376

374377
function withdrawFees(
375378
address token,
376379
uint256 amount,
377380
address recipient
378381
) external override onlyTransferAgent {
382+
require(recipient != address(0), "ERC1450: Invalid recipient");
383+
379384
if (collectedFees[token] < amount) {
380385
revert ERC20InsufficientBalance(address(this), collectedFees[token], amount);
381386
}

contracts/RTAProxy.sol

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ contract RTAProxy {
4343
// Events
4444
event SignerAdded(address indexed signer);
4545
event SignerRemoved(address indexed signer);
46+
event RequiredSignaturesUpdated(uint256 oldRequired, uint256 newRequired);
4647
event OperationSubmitted(uint256 indexed operationId, address indexed submitter);
4748
event OperationConfirmed(uint256 indexed operationId, address indexed signer);
4849
event OperationExecuted(uint256 indexed operationId);
@@ -354,13 +355,15 @@ contract RTAProxy {
354355
* @notice Update required signatures (requires multi-sig approval)
355356
* @dev This should be called through submitOperation
356357
*/
357-
function updateRequiredSignatures(uint256 _requiredSignatures) external {
358+
function updateRequiredSignatures(uint256 newRequiredSignatures) external {
358359
require(msg.sender == address(this), "Must be called through multi-sig");
359360

360-
if (_requiredSignatures == 0 || _requiredSignatures > signers.length) {
361+
if (newRequiredSignatures == 0 || newRequiredSignatures > signers.length) {
361362
revert InvalidSignerCount();
362363
}
363364

364-
requiredSignatures = _requiredSignatures;
365+
uint256 oldRequired = requiredSignatures;
366+
requiredSignatures = newRequiredSignatures;
367+
emit RequiredSignaturesUpdated(oldRequired, newRequiredSignatures);
365368
}
366369
}

test/ERC1450.test.js

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,27 +349,30 @@ describe("ERC1450 Security Token", function () {
349349

350350
describe("Transfer Agent Management", function () {
351351
it("Should allow owner to set initial transfer agent", async function () {
352+
// Deploy with owner as temporary RTA
352353
const newToken = await ERC1450.deploy(
353354
"New Token",
354355
"NEW",
355356
18,
356357
owner.address,
357-
ethers.ZeroAddress
358+
owner.address // Start with owner as RTA
358359
);
359360
await newToken.waitForDeployment();
360361

362+
// Then update to actual RTA
361363
await expect(newToken.setTransferAgent(rta.address))
362364
.to.emit(newToken, "TransferAgentUpdated")
363-
.withArgs(ethers.ZeroAddress, rta.address);
365+
.withArgs(owner.address, rta.address);
364366
});
365367

366368
it("Should lock transfer agent when set to contract", async function () {
369+
// Deploy with owner as temporary RTA
367370
const newToken = await ERC1450.deploy(
368371
"New Token",
369372
"NEW",
370373
18,
371374
owner.address,
372-
ethers.ZeroAddress
375+
owner.address // Start with owner as RTA
373376
);
374377
await newToken.waitForDeployment();
375378

0 commit comments

Comments
 (0)