howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L65-L67

Vulnerability details

Impact

There is a potential Delegate Call vulnerability in the WellUpgradeable contract.

This vulnerability occurs due to the use of delegatecall in the implementation of the proxy contract which has the potential to change the state of the calling contract. The dangerous part about delegatecall is that if the target function isn't careful about what state variables it manipulates or how it manipulates them, it could compromise the calling contract's state.

In this contract, the _authorizeUpgrade() function is using delegatecall to change the state of the current implementation. This could potentially allow an attacker to change the state of the contract in a way that wasn't intended, compromising the contract's security.

Furthermore, the developer needs to understand that delegatecall is potentially unsafe since it gives the target contract complete control over the calling contract's state. The function is obscured into believing that the call was made to itself, and it operates on msg.values as if they were its own. This vulnerability only gets worse if it is combined with another bug or oversights in permissions such as unprotected internal calls or external calls to untrusted contracts.

Proof of Concept

Affected code snippet:

 function _authorizeUpgrade(address newImplmentation) internal view override {
    require(address(this) != ___self, "Function must be called through delegatecall");
    ...
 }

Tools Used

Manual Review

Recommended Mitigation Steps

One way to mitigate the vulnerability is to only allow certain trusted contracts to make delegate calls. This can be done by maintaining a list of trusted contract addresses, and checking the msg.sender before performing a delegate call.

Here is how:

mapping (address => bool) trustedContracts;

function addTrustedContract(address contract) external onlyOwner {
    trustedContracts[contract] = true;
}

function removeTrustedContract(address contract) external onlyOwner {
    trustedContracts[contract] = false;
}

function _authorizeUpgrade(address newImplmentation) internal view override {
    require(trustedContracts[msg.sender], "Caller is not trusted");
    require(address(this) != ___self, "Function must be called through delegatecall");
    ...
}

Furthermore, the best workaround for ERC-1167 minimal proxies is to make them point to another proxy and not to an actual implementation so that the proxy can control the implementation address.

Lastly, the developer should always ensure that functions using delegatecall are properly protected (for example, using an onlyOwner or onlyAdmin modifier), to prevent external malicious contracts from exploiting its mechanism to modify the state of the contract.

Assessed type

call/delegatecall

c4-judge commented 2 months ago

alex-ppg marked the issue as unsatisfactory: Insufficient proof