Refactoring as per comments received on the security audit (#203)

* Remove duplicate modifier on claimTokens method
* Fix multiple reads of totalBurntCoins
* Remove parameter on setInitialize
* Remove status parameter on setFixedAssets
* Remove unused fireEventOnTokenTransfer on HomeBridgeErcToNative
* Redefine empty block methods on BasicHomeBridge
* Simplify signature method on BasicHomeBridge
* Add abi.encodePacked to deployedAtBlock on validators contracts
* Extract fallback implementation into separate method on HomeBridgeErcToNative and HomeBridgeNativeToErc
This commit is contained in:
Gerardo Nardelli 2019-06-28 10:22:05 -03:00 committed by Alexander Kolotov
parent a1f64ca6c5
commit c28770b1f6
15 changed files with 46 additions and 48 deletions

View File

@ -101,7 +101,7 @@ contract BaseBridgeValidators is EternalStorage, Ownable {
}
function deployedAtBlock() public view returns (uint256) {
return uintStorage[keccak256("deployedAtBlock")];
return uintStorage[keccak256(abi.encodePacked("deployedAtBlock"))];
}
function getNextValidator(address _address) public view returns (address) {
@ -120,7 +120,7 @@ contract BaseBridgeValidators is EternalStorage, Ownable {
addressStorage[keccak256(abi.encodePacked("validatorsList", _prevValidator))] = _validator;
}
function setInitialize(bool _status) internal {
boolStorage[keccak256(abi.encodePacked("isInitialized"))] = _status;
function setInitialize() internal {
boolStorage[keccak256(abi.encodePacked("isInitialized"))] = true;
}
}

View File

@ -72,8 +72,8 @@ contract BasicBridge is EternalStorage, Validatable, Ownable, OwnedUpgradeabilit
return uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))];
}
function setInitialize(bool _status) internal {
boolStorage[keccak256(abi.encodePacked("isInitialized"))] = _status;
function setInitialize() internal {
boolStorage[keccak256(abi.encodePacked("isInitialized"))] = true;
}
function isInitialized() public view returns(bool) {

View File

@ -86,11 +86,9 @@ contract BasicHomeBridge is EternalStorage, Validatable {
boolStorage[keccak256(abi.encodePacked("messagesSigned", _hash))] = _status;
}
function onExecuteAffirmation(address, uint256, bytes32) internal returns(bool) {
}
function onExecuteAffirmation(address, uint256, bytes32) internal returns(bool);
function onSignaturesCollected(bytes) internal {
}
function onSignaturesCollected(bytes) internal;
function numAffirmationsSigned(bytes32 _withdrawal) public view returns(uint256) {
return uintStorage[keccak256(abi.encodePacked("numAffirmationsSigned", _withdrawal))];
@ -110,21 +108,13 @@ contract BasicHomeBridge is EternalStorage, Validatable {
function signature(bytes32 _hash, uint256 _index) public view returns (bytes) {
bytes32 signIdx = keccak256(abi.encodePacked(_hash, _index));
return signatures(signIdx);
return bytesStorage[keccak256(abi.encodePacked("signatures", signIdx))];
}
function messagesSigned(bytes32 _message) public view returns(bool) {
return boolStorage[keccak256(abi.encodePacked("messagesSigned", _message))];
}
function messages(bytes32 _hash) internal view returns(bytes) {
return bytesStorage[keccak256(abi.encodePacked("messages", _hash))];
}
function signatures(bytes32 _hash) internal view returns(bytes) {
return bytesStorage[keccak256(abi.encodePacked("signatures", _hash))];
}
function setSignatures(bytes32 _hash, bytes _signature) internal {
bytesStorage[keccak256(abi.encodePacked("signatures", _hash))] = _signature;
}
@ -134,7 +124,7 @@ contract BasicHomeBridge is EternalStorage, Validatable {
}
function message(bytes32 _hash) public view returns (bytes) {
return messages(_hash);
return bytesStorage[keccak256(abi.encodePacked("messages", _hash))];
}
function setNumMessagesSigned(bytes32 _message, uint256 _number) internal {
@ -157,10 +147,7 @@ contract BasicHomeBridge is EternalStorage, Validatable {
return Message.requiredMessageLength();
}
function affirmationWithinLimits(uint256) internal view returns(bool) {
return true;
}
function affirmationWithinLimits(uint256) internal view returns(bool);
function onFailedAffirmation(address, uint256, bytes32) internal {
}
function onFailedAffirmation(address, uint256, bytes32) internal;
}

View File

@ -40,8 +40,8 @@ contract BridgeValidators is BaseBridgeValidators {
}
uintStorage[keccak256(abi.encodePacked("requiredSignatures"))] = _requiredSignatures;
uintStorage[keccak256("deployedAtBlock")] = block.number;
setInitialize(true);
uintStorage[keccak256(abi.encodePacked("deployedAtBlock"))] = block.number;
setInitialize();
emit RequiredSignaturesChanged(_requiredSignatures);
return isInitialized();

View File

@ -20,7 +20,7 @@ contract OverdrawManagement is EternalStorage, OwnedUpgradeability {
if (unlockOnForeign) {
emit UserRequestForSignature(recipient, value);
}
setFixedAssets(txHash, true);
setFixedAssets(txHash);
}
function outOfLimitAmount() public view returns(uint256) {
@ -45,7 +45,7 @@ contract OverdrawManagement is EternalStorage, OwnedUpgradeability {
uintStorage[keccak256(abi.encodePacked("txOutOfLimitValue", _txHash))] = _value;
}
function setFixedAssets(bytes32 _txHash, bool _status) internal {
boolStorage[keccak256(abi.encodePacked("fixedAssets", _txHash))] = _status;
function setFixedAssets(bytes32 _txHash) internal {
boolStorage[keccak256(abi.encodePacked("fixedAssets", _txHash))] = true;
}
}

View File

@ -44,8 +44,8 @@ contract RewardableValidators is BaseBridgeValidators {
}
uintStorage[keccak256(abi.encodePacked("requiredSignatures"))] = _requiredSignatures;
uintStorage[keccak256("deployedAtBlock")] = block.number;
setInitialize(true);
uintStorage[keccak256(abi.encodePacked("deployedAtBlock"))] = block.number;
setInitialize();
emit RequiredSignaturesChanged(_requiredSignatures);
return isInitialized();

View File

@ -32,7 +32,7 @@ contract BasicForeignBridgeErcToErc is BasicBridge, BasicForeignBridge {
uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = _homeDailyLimit;
uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = _homeMaxPerTx;
setOwner(_owner);
setInitialize(true);
setInitialize();
}
function getBridgeMode() public pure returns(bytes4 _data) {

View File

@ -41,7 +41,7 @@ contract HomeBridgeErcToErc is ERC677Receiver, EternalStorage, BasicBridge, Basi
_foreignMaxPerTx,
_owner
);
setInitialize(true);
setInitialize();
return isInitialized();
}
@ -78,7 +78,7 @@ contract HomeBridgeErcToErc is ERC677Receiver, EternalStorage, BasicBridge, Basi
_homeFee,
_foreignFee
);
setInitialize(true);
setInitialize();
return isInitialized();
}

View File

@ -38,7 +38,7 @@ contract POSDAOHomeBridgeErcToErc is HomeBridgeErcToErc {
_foreignFee
);
_setBlockRewardContract(_feeManager, _blockReward);
setInitialize(true);
setInitialize();
return isInitialized();
}

View File

@ -35,7 +35,7 @@ contract ForeignBridgeErcToNative is BasicBridge, BasicForeignBridge {
uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = _homeDailyLimit;
uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = _homeMaxPerTx;
setOwner(_owner);
setInitialize(true);
setInitialize();
return isInitialized();
}

View File

@ -15,12 +15,17 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,
event AmountLimitExceeded(address recipient, uint256 value, bytes32 transactionHash);
function () public payable {
nativeTransfer();
}
function nativeTransfer() internal {
require(msg.value > 0);
require(msg.data.length == 0);
require(withinLimit(msg.value));
IBlockReward blockReward = blockRewardContract();
uint256 totalMinted = blockReward.mintedTotallyByBridge(address(this));
require(msg.value <= totalMinted.sub(totalBurntCoins()));
uint256 totalBurnt = totalBurntCoins();
require(msg.value <= totalMinted.sub(totalBurnt));
setTotalSpentPerDay(getCurrentDay(), totalSpentPerDay(getCurrentDay()).add(msg.value));
uint256 valueToTransfer = msg.value;
address feeManager = feeManagerContract();
@ -30,7 +35,7 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,
valueToTransfer = valueToTransfer.sub(fee);
valueToBurn = getAmountToBurn(valueToBurn);
}
setTotalBurntCoins(totalBurntCoins().add(valueToBurn));
setTotalBurntCoins(totalBurnt.add(valueToBurn));
address(0).transfer(valueToBurn);
emit UserRequestForSignature(msg.sender, valueToTransfer);
}
@ -60,7 +65,7 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,
_foreignMaxPerTx,
_owner
);
setInitialize(true);
setInitialize();
return isInitialized();
}
@ -97,7 +102,7 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,
addressStorage[keccak256(abi.encodePacked("feeManagerContract"))] = _feeManager;
_setFee(_feeManager, _homeFee, HOME_FEE);
_setFee(_feeManager, _foreignFee, FOREIGN_FEE);
setInitialize(true);
setInitialize();
return isInitialized();
}
@ -191,10 +196,6 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,
}
}
function fireEventOnTokenTransfer(address _from, uint256 _value) internal {
emit UserRequestForSignature(_from, _value);
}
function setTotalBurntCoins(uint256 _amount) internal {
uintStorage[keccak256(abi.encodePacked("totalBurntCoins"))] = _amount;
}

View File

@ -38,7 +38,7 @@ contract ForeignBridgeNativeToErc is ERC677Receiver, BasicBridge, BasicForeignBr
_homeMaxPerTx,
_owner
);
setInitialize(true);
setInitialize();
return isInitialized();
}
@ -71,7 +71,7 @@ contract ForeignBridgeNativeToErc is ERC677Receiver, BasicBridge, BasicForeignBr
require(isContract(_feeManager));
addressStorage[keccak256(abi.encodePacked("feeManagerContract"))] = _feeManager;
_setFee(_feeManager, _homeFee, HOME_FEE);
setInitialize(true);
setInitialize();
return isInitialized();
}

View File

@ -11,6 +11,10 @@ import "../Sacrifice.sol";
contract HomeBridgeNativeToErc is EternalStorage, BasicBridge, BasicHomeBridge, RewardableHomeBridgeNativeToErc {
function () public payable {
nativeTransfer();
}
function nativeTransfer() internal {
require(msg.value > 0);
require(msg.data.length == 0);
require(withinLimit(msg.value));
@ -47,7 +51,7 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicBridge, BasicHomeBridge,
_foreignMaxPerTx,
_owner
);
setInitialize(true);
setInitialize();
return isInitialized();
}
@ -81,7 +85,7 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicBridge, BasicHomeBridge,
addressStorage[keccak256(abi.encodePacked("feeManagerContract"))] = _feeManager;
_setFee(_feeManager, _homeFee, HOME_FEE);
_setFee(_feeManager, _foreignFee, FOREIGN_FEE);
setInitialize(true);
setInitialize();
return isInitialized();
}

View File

@ -469,6 +469,9 @@ contract('ForeignBridge_ERC20_to_ERC20', async accounts => {
expect(await tokenSecond.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
expect(await tokenSecond.balanceOf(foreignBridge.address)).to.be.bignumber.equal(halfEther)
await foreignBridge
.claimTokens(tokenSecond.address, accounts[3], { from: accounts[3] })
.should.be.rejectedWith(ERROR_MSG)
await foreignBridge.claimTokens(tokenSecond.address, accounts[3], { from: owner })
expect(await tokenSecond.balanceOf(foreignBridge.address)).to.be.bignumber.equal(ZERO)
expect(await tokenSecond.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)

View File

@ -497,6 +497,9 @@ contract('ForeignBridge_ERC20_to_Native', async accounts => {
expect(await tokenSecond.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
expect(await tokenSecond.balanceOf(foreignBridge.address)).to.be.bignumber.equal(halfEther)
await foreignBridge
.claimTokens(tokenSecond.address, accounts[3], { from: accounts[3] })
.should.be.rejectedWith(ERROR_MSG)
await foreignBridge.claimTokens(tokenSecond.address, accounts[3], { from: owner })
expect(await tokenSecond.balanceOf(foreignBridge.address)).to.be.bignumber.equal(ZERO)
expect(await tokenSecond.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)