Severity: High
Context: Implementation.sol#L18
Since Implementation.sol
is a contract without any access control implemented, anyone who has address for it can simply call it's functions. Hence it's possible for someone to make a delegate call directly from Implementation
contract to a malicious contract which has function which has selfdestruct()
functionality by directly calling the delegatecallContract(address a, bytes calldata _calldata)
function of the Implementation
contract, thus destorying Implementation
contract and rendering the whole Wallet Protocol
as useless.
So an attacker:
- Will create a malicious contract. A very basic one could be:
contract Attack{ constructor () {} function attack() { selfdestruct(); } }
- Will call
delegatecallContract(address a, bytes calldata _calldata)
function at the address ofImplementation
contract with parameters asaddress a = address of Attack contract
andbytes calldata _calldata = function selector for "attack()"
. - Since delegatecall will run the code in context of the caller,
attack()
would be executed in context ofImplementation
and hence destoying it.
Recommendation: We can make "Imeplentation.sol" a library instead of a contract. Since libraries are stateless, it would be perfect to make "Imeplementation.sol" as a library as it must execute the code in caller's context without any scope for storage collision. Also, libraries cannot be destoryed. Since libraries cannot receive/hold ether, we must unmark both of the functions as "payable".
library Implementation { //modified one word -- "contract" --> "library"
function callContract(address a, bytes calldata _calldata) external returns (bytes memory) {
// removed word "payable"
(bool success , bytes memory ret) = a.call{value: msg.value}(_calldata);
require(success);
return ret;
}
function delegatecallContract(address a, bytes calldata _calldata) external returns (bytes memory) {
//removed word "payable"
(bool success, bytes memory ret) = a.delegatecall(_calldata);
require(success);
return ret;
}