diff --git a/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol b/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol index 0fa163180e..22d41fb8e3 100644 --- a/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol +++ b/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol @@ -37,14 +37,6 @@ contract MultiSigWalletWithTimeLock is mapping (uint256 => uint256) public confirmationTimes; - modifier notFullyConfirmed(uint256 transactionId) { - require( - !isConfirmed(transactionId), - "TX_FULLY_CONFIRMED" - ); - _; - } - modifier fullyConfirmed(uint256 transactionId) { require( isConfirmed(transactionId), @@ -93,11 +85,13 @@ contract MultiSigWalletWithTimeLock is ownerExists(msg.sender) transactionExists(transactionId) notConfirmed(transactionId, msg.sender) - notFullyConfirmed(transactionId) { + bool isTxFullyConfirmedBeforeConfirmation = isConfirmed(transactionId); + confirmations[transactionId][msg.sender] = true; emit Confirmation(msg.sender, transactionId); - if (isConfirmed(transactionId)) { + + if (!isTxFullyConfirmedBeforeConfirmation && isConfirmed(transactionId)) { _setConfirmationTime(transactionId, block.timestamp); } } diff --git a/contracts/multisig/test/multi_sig_with_time_lock.ts b/contracts/multisig/test/multi_sig_with_time_lock.ts index 4b85ff9b53..6f59a7f60b 100644 --- a/contracts/multisig/test/multi_sig_with_time_lock.ts +++ b/contracts/multisig/test/multi_sig_with_time_lock.ts @@ -108,26 +108,33 @@ describe('MultiSigWalletWithTimeLock', () => { }); it('should confirm transaction for caller and log a Confirmation event', async () => { const txReceipt = await multiSigWrapper.confirmTransactionAsync(txId, owners[1]); + expect(txReceipt.logs.length).to.equal(2); const log = txReceipt.logs[0] as LogWithDecodedArgs; expect(log.event).to.be.equal('Confirmation'); expect(log.args.sender).to.be.equal(owners[1]); expect(log.args.transactionId).to.be.bignumber.equal(txId); }); - it('should revert if fully confirmed', async () => { - await multiSigWrapper.confirmTransactionAsync(txId, owners[1]); - await expectTransactionFailedAsync( - multiSigWrapper.confirmTransactionAsync(txId, owners[2]), - RevertReason.TxFullyConfirmed, - ); - }); it('should set the confirmation time of the transaction if it becomes fully confirmed', async () => { const txReceipt = await multiSigWrapper.confirmTransactionAsync(txId, owners[1]); + expect(txReceipt.logs.length).to.equal(2); const blockNum = await web3Wrapper.getBlockNumberAsync(); const timestamp = new BigNumber(await web3Wrapper.getBlockTimestampAsync(blockNum)); const log = txReceipt.logs[1] as LogWithDecodedArgs; expect(log.args.confirmationTime).to.be.bignumber.equal(timestamp); expect(log.args.transactionId).to.be.bignumber.equal(txId); }); + it('should confirm transaction for caller but not reset the confirmation time if tx is already fully confirmed', async () => { + await multiSigWrapper.confirmTransactionAsync(txId, owners[1]); + const confirmationTimeBefore = await multiSig.confirmationTimes.callAsync(txId); + const txReceipt = await multiSigWrapper.confirmTransactionAsync(txId, owners[2]); + const confirmationTimeAfter = await multiSig.confirmationTimes.callAsync(txId); + expect(confirmationTimeBefore).to.bignumber.equal(confirmationTimeAfter); + expect(txReceipt.logs.length).to.equal(1); + const log = txReceipt.logs[0] as LogWithDecodedArgs; + expect(log.event).to.be.equal('Confirmation'); + expect(log.args.sender).to.be.equal(owners[2]); + expect(log.args.transactionId).to.be.bignumber.equal(txId); + }); }); describe('executeTransaction', () => { let txId: BigNumber;